runtime: sample large heap allocations correctly #29791
Conversation
Remove an unnecessary check on the heap sampling code that forced sampling of all heap allocations larger than the sampling rate. This need to follow a poisson process so that they can be correctly unsampled. Maintain a check for MemProfileRate==1 to provide a mechanism for full sampling, as documented in https://golang.org/pkg/runtime/#pkg-variables. Additional testing for this change is on cl/129117. Fixes golang#26618
googlebot
added
the
cla: yes
|
This PR (HEAD: d660914) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/158337 to see it. Tip: You can toggle comments from me using the |
|
Message from Raul Silvera: Patch Set 1: This is the one-line branch off cl/129117, for your consideration for 1.12 Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Austin Clements: Patch Set 2: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Hyang-Ah Hana Kim: Patch Set 3: Run-TryBot+1 Code-Review+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Gobot Gobot: Patch Set 3: TryBots beginning. Status page: https://farmer.golang.org/try?commit=e0c40659 Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Austin Clements: Patch Set 3: Bad GerritBot! Raul, can you update the PR to remove the instructions from the commit message? I tried to, but GerritBot just put it back. Otherwise this change LGTM. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Gobot Gobot: Patch Set 3: Build is still in progress... Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Gobot Gobot: Patch Set 3: TryBot-Result-1 6 of 19 TryBots failed: Consult https://build.golang.org/ to see whether they are new failures. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Raul Silvera: Patch Set 4: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Hyang-Ah Hana Kim: Patch Set 5:
It seems like a real failure - maybe for the test we should make sure we set the profile rate to 1. Large memory allocation sampling is no longer deterministic. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Austin Clements: Patch Set 5: Okay, the issue with the commit message is golang.org/issue/25359. Raul, could you try editing your PR's description, rather than the commit message itself? E.g., edit the first message under the "Conversation" at #29791. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Austin Clements: Patch Set 6: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Gobot Gobot: Patch Set 6: TryBots beginning. Status page: https://farmer.golang.org/try?commit=0dee4dea Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Gobot Gobot: Patch Set 6: Build is still in progress... Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Gobot Gobot: Patch Set 6: TryBot-Result-1 6 of 19 TryBots failed: Consult https://build.golang.org/ to see whether they are new failures. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
This PR (HEAD: 7d34aff) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/158337 to see it. Tip: You can toggle comments from me using the |
|
Message from Austin Clements: Patch Set 7: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
Also reduce the number of allocations in this testcase, as it no longer requires many allocations to trigger sampling.
|
Message from Raul Silvera: Patch Set 7: (1 comment)
Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
This PR (HEAD: 471f747) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/158337 to see it. Tip: You can toggle comments from me using the |
|
Message from Austin Clements: Patch Set 8: Run-TryBot+1 I see. Thanks! Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Gobot Gobot: Patch Set 8: TryBots beginning. Status page: https://farmer.golang.org/try?commit=a5e96756 Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Gobot Gobot: Patch Set 8: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
|
Message from Hyang-Ah Hana Kim: Patch Set 8: Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/158337. |
Remove an unnecessary check on the heap sampling code that forced sampling of all heap allocations larger than the sampling rate. This need to follow a poisson process so that they can be correctly unsampled. Maintain a check for MemProfileRate==1 to provide a mechanism for full sampling, as documented in https://golang.org/pkg/runtime/#pkg-variables. Additional testing for this change is on cl/129117. Fixes #26618 Change-Id: I7802bde2afc655cf42cffac34af9bafeb3361957 GitHub-Last-Rev: 471f747 GitHub-Pull-Request: #29791 Reviewed-on: https://go-review.googlesource.com/c/158337 Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
|
This PR is being closed because golang.org/cl/158337 has been merged. |
Remove an unnecessary check on the heap sampling code that forced sampling
of all heap allocations larger than the sampling rate. This need to follow
a poisson process so that they can be correctly unsampled. Maintain a check
for MemProfileRate==1 to provide a mechanism for full sampling, as
documented in https://golang.org/pkg/runtime/#pkg-variables.
Additional testing for this change is on cl/129117.
Fixes #26618