Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1706343002:
Unsampling for the sampling heap profiler (Closed)
|
Created:
4 years, 10 months ago by mattloring Modified:
4 years, 10 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionUnsampling for the sampling heap profiler
Implements poisson unsampling. A poisson process is used to determine
which samples to collect based on a sample rate. Unsampling will
approximate the true number of allocations at each site taking into
account that smaller allocations are less likley to be sampled.
This work was originally being done in the agent that
consumes profiles but it is more efficient to do it here
and individual consumers of the API should not have to
worry about the mathematical details of the sampling
process.
R=ofrobots@google.com
BUG=
Committed: https://crrev.com/50537badae90c5ab5bd2c5b19a22f87534c61a90
Cr-Commit-Position: refs/heads/master@{#34234}
Patch Set 1 #Patch Set 2 : Comment describing poisson unsampling #
Total comments: 4
Patch Set 3 : Add comment on rounding #
Total comments: 2
Patch Set 4 : Assert rate > 0 #Patch Set 5 : Different sampling frequencies should give similar allocation counts #Patch Set 6 : Elaborated on unsampling in comments #Patch Set 7 : Compute percent change in tests #
Total comments: 4
Patch Set 8 : Adjust sampling rates in comparatives rate test #
Messages
Total messages: 47 (18 generated)
Relies on the re-landing of https://codereview.chromium.org/1697903002/.
https://codereview.chromium.org/1706343002/diff/20001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1706343002/diff/20001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:46: return {size, static_cast<unsigned int>(count * scale + 0.5)}; Where did the 0.5 come from?
https://codereview.chromium.org/1706343002/diff/20001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1706343002/diff/20001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:46: return {size, static_cast<unsigned int>(count * scale + 0.5)}; On 2016/02/19 04:29:29, ofrobots wrote: > Where did the 0.5 come from? Thought it would be more accurate to round than truncate. Happy to go with truncating if that is the desired behavior.
https://codereview.chromium.org/1706343002/diff/20001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1706343002/diff/20001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:46: return {size, static_cast<unsigned int>(count * scale + 0.5)}; On 2016/02/19 04:31:07, mattloring wrote: > On 2016/02/19 04:29:29, ofrobots wrote: > > Where did the 0.5 come from? > > Thought it would be more accurate to round than truncate. Happy to go with > truncating if that is the desired behavior. Ah. This deserves a comment in the code.
https://codereview.chromium.org/1706343002/diff/20001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1706343002/diff/20001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:46: return {size, static_cast<unsigned int>(count * scale + 0.5)}; On 2016/02/19 04:46:43, ofrobots wrote: > On 2016/02/19 04:31:07, mattloring wrote: > > On 2016/02/19 04:29:29, ofrobots wrote: > > > Where did the 0.5 come from? > > > > Thought it would be more accurate to round than truncate. Happy to go with > > truncating if that is the desired behavior. > > Ah. This deserves a comment in the code. Done.
https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:45: double scale = 1.0 / (1.0 - std::exp(-size / rate_)); Need an assertion (probably in the constructor, perhaps earlier in the v8-profiler API) that rate is non-zero to avoid a division by zero here.
https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.cc:45: double scale = 1.0 / (1.0 - std::exp(-size / rate_)); On 2016/02/19 05:01:27, ofrobots wrote: > Need an assertion (probably in the constructor, perhaps earlier in the > v8-profiler API) that rate is non-zero to avoid a division by zero here. What form do assertions take here? Is that the same as DCHECK?
On 2016/02/19 05:16:10, mattloring wrote: > https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... > File src/profiler/sampling-heap-profiler.cc (right): > > https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... > src/profiler/sampling-heap-profiler.cc:45: double scale = 1.0 / (1.0 - > std::exp(-size / rate_)); > On 2016/02/19 05:01:27, ofrobots wrote: > > Need an assertion (probably in the constructor, perhaps earlier in the > > v8-profiler API) that rate is non-zero to avoid a division by zero here. > > What form do assertions take here? Is that the same as DCHECK?
On 2016/02/19 05:16:39, ofrobots wrote: > On 2016/02/19 05:16:10, mattloring wrote: > > > https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... > > File src/profiler/sampling-heap-profiler.cc (right): > > > > > https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... > > src/profiler/sampling-heap-profiler.cc:45: double scale = 1.0 / (1.0 - > > std::exp(-size / rate_)); > > On 2016/02/19 05:01:27, ofrobots wrote: > > > Need an assertion (probably in the constructor, perhaps earlier in the > > > v8-profiler API) that rate is non-zero to avoid a division by zero here. > > > > What form do assertions take here? Is that the same as DCHECK? Yes CHECK/DCHECK. This one should be a CHECK however.
On 2016/02/19 05:17:05, ofrobots wrote: > On 2016/02/19 05:16:39, ofrobots wrote: > > On 2016/02/19 05:16:10, mattloring wrote: > > > > > > https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... > > > File src/profiler/sampling-heap-profiler.cc (right): > > > > > > > > > https://codereview.chromium.org/1706343002/diff/40001/src/profiler/sampling-h... > > > src/profiler/sampling-heap-profiler.cc:45: double scale = 1.0 / (1.0 - > > > std::exp(-size / rate_)); > > > On 2016/02/19 05:01:27, ofrobots wrote: > > > > Need an assertion (probably in the constructor, perhaps earlier in the > > > > v8-profiler API) that rate is non-zero to avoid a division by zero here. > > > > > > What form do assertions take here? Is that the same as DCHECK? > > Yes CHECK/DCHECK. This one should be a CHECK however. Done.
Description was changed from ========== Unsampling for the sampling heap profiler Implements poisson unsampling. A poisson process is used to determine which samples to collect based on a sample rate. Unsampling will approximate the true number of allocations at each site taking into account that smaller allocations are less likley to be sampled. R=ofrobots@google.com BUG= ========== to ========== Unsampling for the sampling heap profiler Implements poisson unsampling. A poisson process is used to determine which samples to collect based on a sample rate. Unsampling will approximate the true number of allocations at each site taking into account that smaller allocations are less likley to be sampled. This work was originally being done in the agent that consumes profiles but it is more efficient to do it here and individual consumers of the API should not have to worry about the mathematical details of the sampling process. R=ofrobots@google.com BUG= ==========
Description was changed from ========== Unsampling for the sampling heap profiler Implements poisson unsampling. A poisson process is used to determine which samples to collect based on a sample rate. Unsampling will approximate the true number of allocations at each site taking into account that smaller allocations are less likley to be sampled. This work was originally being done in the agent that consumes profiles but it is more efficient to do it here and individual consumers of the API should not have to worry about the mathematical details of the sampling process. R=ofrobots@google.com BUG= ========== to ========== Unsampling for the sampling heap profiler Implements poisson unsampling. A poisson process is used to determine which samples to collect based on a sample rate. Unsampling will approximate the true number of allocations at each site taking into account that smaller allocations are less likley to be sampled. This work was originally being done in the agent that consumes profiles but it is more efficient to do it here and individual consumers of the API should not have to worry about the mathematical details of the sampling process. R=ofrobots@google.com BUG= ==========
mattloring@google.com changed reviewers: + rsilvera@google.com
ofrobots@google.com changed reviewers: + alph@chromium.org, ulan@chromium.org
On 2016/02/22 17:00:38, ofrobots wrote: > mailto:ofrobots@google.com changed reviewers: > + mailto:alph@chromium.org, mailto:ulan@chromium.org lgtm.
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706343002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706343002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/3287)
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706343002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706343002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706343002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706343002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706343002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706343002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/3291) v8_win64_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
lgtm https://codereview.chromium.org/1706343002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1706343002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:43: v8::AllocationProfile::Allocation SamplingHeapProfiler::Unsample( How about naming it NormalizeCount?
lgtm https://codereview.chromium.org/1706343002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1706343002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:65: CHECK(rate_ > 0); CHECK_GT
https://codereview.chromium.org/1706343002/diff/120001/src/profiler/sampling-... File src/profiler/sampling-heap-profiler.cc (right): https://codereview.chromium.org/1706343002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:43: v8::AllocationProfile::Allocation SamplingHeapProfiler::Unsample( On 2016/02/23 08:37:59, alph wrote: > How about naming it NormalizeCount? I've switched over to ScaleSample to stay consistent with the Go profiler: https://github.com/golang/go/blob/master/src/cmd/pprof/internal/profile/legac... https://codereview.chromium.org/1706343002/diff/120001/src/profiler/sampling-... src/profiler/sampling-heap-profiler.cc:65: CHECK(rate_ > 0); On 2016/02/23 08:44:12, ulan wrote: > CHECK_GT Done.
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706343002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706343002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mattloring@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ofrobots@google.com, alph@chromium.org, ulan@chromium.org Link to the patchset: https://codereview.chromium.org/1706343002/#ps140001 (title: "Adjust sampling rates in comparatives rate test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706343002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706343002/140001
Message was sent while issue was closed.
Description was changed from ========== Unsampling for the sampling heap profiler Implements poisson unsampling. A poisson process is used to determine which samples to collect based on a sample rate. Unsampling will approximate the true number of allocations at each site taking into account that smaller allocations are less likley to be sampled. This work was originally being done in the agent that consumes profiles but it is more efficient to do it here and individual consumers of the API should not have to worry about the mathematical details of the sampling process. R=ofrobots@google.com BUG= ========== to ========== Unsampling for the sampling heap profiler Implements poisson unsampling. A poisson process is used to determine which samples to collect based on a sample rate. Unsampling will approximate the true number of allocations at each site taking into account that smaller allocations are less likley to be sampled. This work was originally being done in the agent that consumes profiles but it is more efficient to do it here and individual consumers of the API should not have to worry about the mathematical details of the sampling process. R=ofrobots@google.com BUG= ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Unsampling for the sampling heap profiler Implements poisson unsampling. A poisson process is used to determine which samples to collect based on a sample rate. Unsampling will approximate the true number of allocations at each site taking into account that smaller allocations are less likley to be sampled. This work was originally being done in the agent that consumes profiles but it is more efficient to do it here and individual consumers of the API should not have to worry about the mathematical details of the sampling process. R=ofrobots@google.com BUG= ========== to ========== Unsampling for the sampling heap profiler Implements poisson unsampling. A poisson process is used to determine which samples to collect based on a sample rate. Unsampling will approximate the true number of allocations at each site taking into account that smaller allocations are less likley to be sampled. This work was originally being done in the agent that consumes profiles but it is more efficient to do it here and individual consumers of the API should not have to worry about the mathematical details of the sampling process. R=ofrobots@google.com BUG= Committed: https://crrev.com/50537badae90c5ab5bd2c5b19a22f87534c61a90 Cr-Commit-Position: refs/heads/master@{#34234} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/50537badae90c5ab5bd2c5b19a22f87534c61a90 Cr-Commit-Position: refs/heads/master@{#34234}
Message was sent while issue was closed.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
