New issue
Advanced search Search tips

Issue 905379 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 19
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Some LegacyCallStackProfileBuilder samples contain frames from previous stacks

Project Member Reported by wittman@chromium.org, Nov 14

Issue description

The current sample is being cleared only in cases where the stack is not deduplicated. It should be cleared in both deduplicated and non-deduplicated cases. This is resulting in frames from previous deduplicated samples incorrectly being added to subsequent samples.
 
LegacyCallStackProfileBuilder? Was the issue introduced by my CLs?
I suspect it was introduced at some point during the refactoring. I still need to analyze the impact of this bug on the resulting data and should be able to pin it down in that process.

I noticed this when analyzing data sizes and was surprised to find that the new profiles were smaller than the old ones. :)
Oops, sorry about that :-)
No worries. This ideally would have been caught by server-side alerting, but we don't have that in place yet.

It looks like this was introduced in 70.0.3523.0 with e91f6fdb3d1b31b3c9ca8370fcab83a290118358. The effect is that 9% of non-idle samples were treated as idle[1]. These were almost exclusively samples seen in work done from the message loop, so this didn't significantly affect the pre-message loop startup measurement which is substantially what we care about for startup regression detection.

To the extent this introduced bias -- beyond just missing non-idle samples -- it would have been against non-idle samples seen immediately after idle samples.

[1] https://uma.googleplex.com/p/chrome/callstacks?sid=812677caa3d4e349dee574dd82af7515 demonstrates this shift pretty clearly.
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/311a3e55aa4ddc50db1049b6af0776b94d77517a

commit 311a3e55aa4ddc50db1049b6af0776b94d77517a
Author: Mike Wittman <wittman@chromium.org>
Date: Wed Nov 14 23:12:35 2018

[Sampling profiler] Fix incorrect reuse of frames across samples

Frames from deduplicated samples were being left in the current sample
state on the class, resulting in incorrect extra frames being added when
the next sample was recorded. This change eliminates the current sample
state to avoid this problem, and adds a test for this case.

Bug:  905379 
Change-Id: I5fe0ccd3e50675669d4c7124fb7f64ebb85fe807
Reviewed-on: https://chromium-review.googlesource.com/c/1336247
Reviewed-by: Xi Cheng <chengx@chromium.org>
Commit-Queue: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608160}
[modify] https://crrev.com/311a3e55aa4ddc50db1049b6af0776b94d77517a/components/metrics/legacy_call_stack_profile_builder.cc
[modify] https://crrev.com/311a3e55aa4ddc50db1049b6af0776b94d77517a/components/metrics/legacy_call_stack_profile_builder.h
[modify] https://crrev.com/311a3e55aa4ddc50db1049b6af0776b94d77517a/components/metrics/legacy_call_stack_profile_builder_unittest.cc

Status: Fixed (was: Started)
The fix went in to 72.0.3611.0 and the generated profiles from that version look good.

Sign in to add a comment