cmd/compile: 1.15 heap allocations regression when calling Write on os.Stdout #41474
Comments
ALTree
added
the
NeedsInvestigation
|
I think that commit is indeed the cause. Looking at os/file.go, these two: Building at 8c1db77 gives: building at its parent: |
|
Probably we need to mark the (But that's a bit unfortunate, because that function is actually parametrically |
|
Well, that's annoying.
I'll let the compiler team decide whether they want to improve the escape analysis or whether we should replace CC @dr2chase |
|
The escape analysis may be failing in this case because the functions are not being inlined. The functions cannot be inlined because they contain loops (that's #14768). |
|
Clearly we should replace the loops with |
|
I guess by using |
|
Looks like we don't get around to that constant-fold: This is sad. |
|
Looks like we do have a few opportunities to do that. |
|
Change https://golang.org/cl/256077 mentions this issue: |
|
Changing inlining to allow |
|
Inlining |
|
We should find and fix that bug. Inlining |
|
I think I found the issue. |
|
Okay, I found a sequence of optimizations that makes the original code no longer heap escape:
It's worth noting that the last 3 steps are only necessary because ignoringEINTR accepts a function and the arguments to pass to it. If it's changed so that call sites look like: then optimization 1 alone is sufficient. This is because it's possible for escape analysis to tell that |
On tip this is |
|
Oh, I missed that this is a regression with Go 1.15, not tip, so fixing it requires backporting changes. I'd rather not have to backport any new inlining / escape analysis optimizations. Also, correction: if we change the function to take a simple closure, then I think none of the optimizations are strictly necessary. I think that change would be best/safest to backport to Go 1.15. I'll still upload CLs for the optimizations though, because they look like they'll help optimize a fair bit of cmd code. |
|
@gopherbot Please open a backport issue for 1.15. This is an unexpected and unavoidable performance regression in 1.15: using |
|
Backport issue(s) opened: #41542 (for 1.14), #41543 (for 1.15). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
|
Change https://golang.org/cl/256418 mentions this issue: |
|
Change https://golang.org/cl/256460 mentions this issue: |
|
Change https://golang.org/cl/256458 mentions this issue: |
|
Change https://golang.org/cl/256457 mentions this issue: |
|
Change https://golang.org/cl/256459 mentions this issue: |
…slice escape The 1.15 compiler is not quite smart enough to see that the byte slice passed to ignoringEINTR does not escape. This ripples back up to user code which would see a byte slice passed to os.(*File).Write escape, which did not happen in 1.14. Rather than backport some moderately complex compiler fixes, rewrite the code slightly so that the 1.15 compiler is able to see that the slice does not escape. This is not a backport from tip, where the code is already different. The test for this will be on tip, where we will most likely change the compiler to understand this kind of code. Fixes #41543 For #41474 Change-Id: I6c78164229fea7794e7edba512bfd7034a0b91c3 Reviewed-on: https://go-review.googlesource.com/c/go/+/256418 Trust: Ian Lance Taylor <iant@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
|
Given that we've backported a fix for this issue to Go 1.15, I believe we should not release Go 1.16 without fixing this (or re-visiting this in some way), so marking as a release-blocker. |
|
The plan here is to try to fix this in the compiler, but if we get to the freeze without that landing, we'll just forward-port the 1.15 fix. |
Normally, when variables are declared and initialized using ":=", we set the variable's n.Name.Defn to point to the initialization assignment node (i.e., OAS or OAS2). Further, some frontend optimizations look for variables that are initialized but never reassigned. However, when inl.go inlines calls, it was declaring the inlined variables, and then separately assigning to them. This CL changes inl.go tweaks the AST to fit the combined declaration+initialization pattern. This isn't terribly useful by itself, but it allows further followup optimizations. Updates #41474. Change-Id: I62a9752c60414305679e0ed15a6563baa0224efa Reviewed-on: https://go-review.googlesource.com/c/go/+/256457 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Go Bot <gobot@golang.org> Trust: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Escape analysis is currently very naive about identifying calls to known functions: it only recognizes direct calls to a declared function, or direct calls to a closure. This CL adds a new "staticValue" helper function that can trace back through local variables that were initialized and never reassigned based on a similar optimization already used by inlining. (And to be used by inlining in a followup CL.) Updates #41474. Change-Id: I8204fd3b1e150ab77a27f583985cf099a8572b2e Reviewed-on: https://go-review.googlesource.com/c/go/+/256458 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Go Bot <gobot@golang.org> Trust: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: David Chase <drchase@google.com>
This CL replaces the ad hoc and duplicated logic for detecting inlinable calls with a single "inlCallee" function, which uses the "staticValue" helper function introduced in an earlier commit. Updates #41474. Change-Id: I103d4091b10366fce1344ef2501222b7df68f21d Reviewed-on: https://go-review.googlesource.com/c/go/+/256460 Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Trust: Matthew Dempsky <mdempsky@google.com>
|
Change https://golang.org/cl/262678 mentions this issue: |
|
I've landed all the individual compiler optimizations to fix this, but it's proving tedious to write a test that shows the original test case involving os.Stdout.Write has actually been fixed. E.g., js/wasm and apparently Windows also still escape, but at least js/wasm also escaped in Go 1.15. I haven't looked into Windows. It should be a straightforward task for someone to take CL 262678 and amend it. Probably it just needs to check |
|
|
|
Change https://golang.org/cl/263097 mentions this issue: |
This issue was fixed with multiple individual compiler optimizations, each of which had their own respective test cases. This CL just adds the capstone test case to demonstrate that the issue has been fixed and doesn't regress again. Updates #41474. Change-Id: Iae752d4b0e7b83ee356b946843340a4fbc254058 Reviewed-on: https://go-review.googlesource.com/c/go/+/263097 Trust: Alberto Donizetti <alb.donizetti@gmail.com> Trust: Ian Lance Taylor <iant@golang.org> Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
|
Thanks @ALTree for helping with the test case! |
|
Change https://golang.org/cl/263537 mentions this issue: |
This reverts CL 263097. Reason for revert: broke the noopt builder. Change-Id: Ie36d2c3ed9449b4425732072db624c8e18f965f3 Reviewed-on: https://go-review.googlesource.com/c/go/+/263537 Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Than McIntosh <thanm@google.com> Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Go Bot <gobot@golang.org> Trust: Alberto Donizetti <alb.donizetti@gmail.com>
…slice escape The 1.15 compiler is not quite smart enough to see that the byte slice passed to ignoringEINTR does not escape. This ripples back up to user code which would see a byte slice passed to os.(*File).Write escape, which did not happen in 1.14. Rather than backport some moderately complex compiler fixes, rewrite the code slightly so that the 1.15 compiler is able to see that the slice does not escape. This is not a backport from tip, where the code is already different. The test for this will be on tip, where we will most likely change the compiler to understand this kind of code. Fixes golang#41543 For golang#41474 Change-Id: I6c78164229fea7794e7edba512bfd7034a0b91c3 Reviewed-on: https://go-review.googlesource.com/c/go/+/256418 Trust: Ian Lance Taylor <iant@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
dmitshur
added
NeedsFix
I have a program that shall not heap-allocate in its main loop, with a test checking that. When I re-compiled the program with Go1.15, the test started failing. This is the offending line according to mem profile data:
Ignore the string literal cast to
[]byte, it's a red herring. Here's a reproducer:Go1.14:
Go1.15:
git bisectpoints to 8c1db77, but I don't know if that makes sense.cc @dr2chase @randall77 @ianlancetaylor
The text was updated successfully, but these errors were encountered: