Closed Bug 1434965 Opened 6 years ago Closed 6 years ago

UniqueStacks holds on to the JSContext pointer for too long

Categories

(Core :: Gecko Profiler, defect, P1)

Product:
Core
Component:
Gecko Profiler
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(11 files)

Bug 1434965 - Replace callback-based API ForEachProfiledFrame with an iterator-based API called GetProfiledFrames.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Remove Stack class and use intended-to-be-immutable StackKeys.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Split StreamFrame into two different implementations for JIT and non-JIT frames.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Remove OnStackFrameKey and solve frame canonicalization differently.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Remove AutoArraySchemaWriter constructor that doesn't take a string table.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Add AutoArraySchemaWriter::FreeFormElement so that FillUpTo can be private.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Create an implementation of JS::ForEachTrackedOptimizationAttemptOp which takes a lambda.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Add ForEachTrackedOptimizationTypeInfoLambdaOp.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Eliminate custom StringKey class by using nsCStringHashKey.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Don't store a JSContext pointer in UniqueStacks; pass it manually to the functions that need it.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1434965 - Take 'streaming generation' into account when de-duplicating JIT frames.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
When a thread loses its JSContext, ThreadInfo::FlushSamplesAndMarkers is called and passes the JSContext to mUniqueStacks just before the context is nulled out:

  // Note that the UniqueStacks instance is persisted so that the frame-index
  // mapping is stable across JS shutdown.
  mUniqueStacks.emplace(mContext);

The expectation is that we'll never profile any JS from here on out on this thread again. If we later dump the profile, ThreadInfo::StreamJSON will re-use the existing mUniqueStacks object, but since there will be no JS entries in the buffer for that thread, the JSContext pointer stored on mUniqueStacks will not be accessed.

However, now that we have DOM worker threads registered for the entire lifetime of the OS thread, this assumption is invalidated: After losing its JSContext for one worker script, once another script is run on the thread, there will be a new JSContext. Here's where we set and clear the JSContext for worker threads:
https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/dom/workers/RuntimeService.cpp#2734,2748

So if we flush samples with one JSContext, then profile some more with a new JSContext, and then stream the samples, we'll try to interpret JS entries in the buffer from the new JSContext using the old JSContext that's stored on mUniqueStacks. That seems bad.
Keywords: regression
Blocks: 1337558
Depends on: 1436924
Priority: -- → P1
There were really only two small problems here:
 1. UniqueStacks::StreamFrame() was using mContext when streaming optimization info, and mContext might already have been dead. Capturing of optimization info is off by default, so this was probably never hit in practice.
 2. Frame de-duplication from two streaming attempts could have treated different frames as the same frame if the frame's JIT return address happened to be the same (through re-allocation of JIT code after JSContext destruction).

Most of the patches are cleanup.
Comment on attachment 8950031 [details]
Bug 1434965 - Create an implementation of JS::ForEachTrackedOptimizationAttemptOp which takes a lambda.

https://reviewboard.mozilla.org/r/219322/#review225030


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: tools/profiler/core/ProfileBufferEntry.cpp:240
(Diff revision 1)
>  {
> -  SpliceableJSONWriter& mWriter;
> -  UniqueJSONStrings& mUniqueStrings;
> -
>  public:
> -  StreamOptimizationAttemptsOp(SpliceableJSONWriter& aWriter, UniqueJSONStrings& aUniqueStrings)
> +  ForEachTrackedOptimizationAttemptsLambdaOp(LambdaT&& aLambda)

Error: Bad implicit conversion constructor for 'foreachtrackedoptimizationattemptslambdaop' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8950032 [details]
Bug 1434965 - Add ForEachTrackedOptimizationTypeInfoLambdaOp.

https://reviewboard.mozilla.org/r/219324/#review225032


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: tools/profiler/core/ProfileBufferEntry.cpp:119
(Diff revision 1)
>  public:
> -  StreamOptimizationTypeInfoOp(JSONWriter& aWriter, UniqueJSONStrings& aUniqueStrings)
> -    : mWriter(aWriter)
> -    , mUniqueStrings(aUniqueStrings)
> -    , mStartedTypeList(false)
> -  { }
> +  // aLambda needs to be a function with the following signature:
> +  // void lambda(JS::TrackedTypeSite site, const char* mirType,
> +  //             const nsTArray<TypeInfo>& typeset)
> +  // aLambda will be called once per entry.
> +  ForEachTrackedOptimizationTypeInfoLambdaOp(LambdaT&& aLambda)

Error: Bad implicit conversion constructor for 'foreachtrackedoptimizationtypeinfolambdaop' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8950031 [details]
Bug 1434965 - Create an implementation of JS::ForEachTrackedOptimizationAttemptOp which takes a lambda.

https://reviewboard.mozilla.org/r/219322/#review225034


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: tools/profiler/core/ProfileBufferEntry.cpp:240
(Diff revision 2)
>  {
> -  SpliceableJSONWriter& mWriter;
> -  UniqueJSONStrings& mUniqueStrings;
> -
>  public:
> -  StreamOptimizationAttemptsOp(SpliceableJSONWriter& aWriter, UniqueJSONStrings& aUniqueStrings)
> +  ForEachTrackedOptimizationAttemptsLambdaOp(LambdaT&& aLambda)

Error: Bad implicit conversion constructor for 'foreachtrackedoptimizationattemptslambdaop' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8950032 [details]
Bug 1434965 - Add ForEachTrackedOptimizationTypeInfoLambdaOp.

https://reviewboard.mozilla.org/r/219324/#review225036


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: tools/profiler/core/ProfileBufferEntry.cpp:119
(Diff revision 2)
>  public:
> -  StreamOptimizationTypeInfoOp(JSONWriter& aWriter, UniqueJSONStrings& aUniqueStrings)
> -    : mWriter(aWriter)
> -    , mUniqueStrings(aUniqueStrings)
> -    , mStartedTypeList(false)
> -  { }
> +  // aLambda needs to be a function with the following signature:
> +  // void lambda(JS::TrackedTypeSite site, const char* mirType,
> +  //             const nsTArray<TypeInfo>& typeset)
> +  // aLambda will be called once per entry.
> +  ForEachTrackedOptimizationTypeInfoLambdaOp(LambdaT&& aLambda)

Error: Bad implicit conversion constructor for 'foreachtrackedoptimizationtypeinfolambdaop' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8950025 [details]
Bug 1434965 - Replace callback-based API ForEachProfiledFrame with an iterator-based API called GetProfiledFrames.

https://reviewboard.mozilla.org/r/219310/#review225102
Attachment #8950025 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950026 [details]
Bug 1434965 - Remove Stack class and use intended-to-be-immutable StackKeys.

https://reviewboard.mozilla.org/r/219312/#review225104
Attachment #8950026 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950027 [details]
Bug 1434965 - Split StreamFrame into two different implementations for JIT and non-JIT frames.

https://reviewboard.mozilla.org/r/219314/#review225100

::: tools/profiler/core/ProfileBufferEntry.cpp:362
(Diff revision 2)
> +  }
> +
> +  index = mFrameCount++;
> +  mFrameToIndexMap.Put(aFrame, index);
> +  StreamNonJITFrame(aFrame);
>    return index;

I would put these last few lines in an `else` block, because they have equal logical weight as the `if` block. And `return index` could still be at the end.
Attachment #8950027 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950028 [details]
Bug 1434965 - Remove OnStackFrameKey and solve frame canonicalization differently.

https://reviewboard.mozilla.org/r/219316/#review225106
Attachment #8950028 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950029 [details]
Bug 1434965 - Remove AutoArraySchemaWriter constructor that doesn't take a string table.

https://reviewboard.mozilla.org/r/219318/#review225108
Attachment #8950029 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950030 [details]
Bug 1434965 - Add AutoArraySchemaWriter::FreeFormElement so that FillUpTo can be private.

https://reviewboard.mozilla.org/r/219320/#review225112
Attachment #8950030 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950031 [details]
Bug 1434965 - Create an implementation of JS::ForEachTrackedOptimizationAttemptOp which takes a lambda.

https://reviewboard.mozilla.org/r/219322/#review225114
Attachment #8950031 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950032 [details]
Bug 1434965 - Add ForEachTrackedOptimizationTypeInfoLambdaOp.

https://reviewboard.mozilla.org/r/219324/#review225116
Attachment #8950032 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950033 [details]
Bug 1434965 - Eliminate custom StringKey class by using nsCStringHashKey.

https://reviewboard.mozilla.org/r/219326/#review225110

Nice.
Attachment #8950033 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950034 [details]
Bug 1434965 - Don't store a JSContext pointer in UniqueStacks; pass it manually to the functions that need it.

https://reviewboard.mozilla.org/r/219328/#review225120
Attachment #8950034 - Flags: review?(n.nethercote) → review+
Comment on attachment 8950035 [details]
Bug 1434965 - Take 'streaming generation' into account when de-duplicating JIT frames.

https://reviewboard.mozilla.org/r/219330/#review225122
Attachment #8950035 - Flags: review?(n.nethercote) → review+
FWIW, I feel like my reviews here didn't add much because you touched a lot of code that I haven't interacted with much... hopefully you're confident with the changes :)
Thanks for the reviews! I'm fairly confident in these patches.

I wasn't very familiar with this code until last week either, but then on Thursday night, I decided it was time for me to understand it. So I printed out all 25 pages of ProfileBufferEntry.h/.cpp and worked through them with a pen, and formed some opinions. After these patches, I'm mostly happy with the state of the code.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/b99f2166a10b
Replace callback-based API ForEachProfiledFrame with an iterator-based API called GetProfiledFrames. r=njn
https://hg.mozilla.org/integration/autoland/rev/af243ffe2e85
Remove Stack class and use intended-to-be-immutable StackKeys. r=njn
https://hg.mozilla.org/integration/autoland/rev/51b62e61e969
Split StreamFrame into two different implementations for JIT and non-JIT frames. r=njn
https://hg.mozilla.org/integration/autoland/rev/19ee2c8eb9a4
Remove OnStackFrameKey and solve frame canonicalization differently. r=njn
https://hg.mozilla.org/integration/autoland/rev/b3fdcf1aabe3
Remove AutoArraySchemaWriter constructor that doesn't take a string table. r=njn
https://hg.mozilla.org/integration/autoland/rev/ffa3422ee9e2
Add AutoArraySchemaWriter::FreeFormElement so that FillUpTo can be private. r=njn
https://hg.mozilla.org/integration/autoland/rev/7d482ab2b5b8
Create an implementation of JS::ForEachTrackedOptimizationAttemptOp which takes a lambda. r=njn
https://hg.mozilla.org/integration/autoland/rev/31a366f730ab
Add ForEachTrackedOptimizationTypeInfoLambdaOp. r=njn
https://hg.mozilla.org/integration/autoland/rev/cf5aea056d7e
Eliminate custom StringKey class by using nsCStringHashKey. r=njn
https://hg.mozilla.org/integration/autoland/rev/c074ed11b5dc
Don't store a JSContext pointer in UniqueStacks; pass it manually to the functions that need it. r=njn
https://hg.mozilla.org/integration/autoland/rev/749e2fa440ac
Take 'streaming generation' into account when de-duplicating JIT frames. r=njn
Assignee: nobody → mstange
Status: NEW → ASSIGNED
It seems like this improved one of our microbenchmark tests. I'm not too sure about it though, given the lack of stability of these tests.

== Change summary for alert #11571 (as of Mon, 12 Feb 2018 19:13:08 GMT) ==

Improvements:

  4%  Strings PerfHasRTLCharsJA windows7-32 opt      355,423.00 -> 341,956.25

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11571
I think it's very unlikely that the PerfHasRTLCharsJA performance improvement is related to the patches from this bug.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: