Closed
Bug 1434965
Opened 7 years ago
Closed 7 years ago
UniqueStacks holds on to the JSContext pointer for too long
Categories
(Core :: Gecko Profiler, defect, P1)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla60
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)
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
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.
Assignee | ||
Updated•7 years ago
|
Keywords: regression
Updated•7 years ago
|
Updated•7 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•7 years ago
|
||
mozreview-review |
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 25•7 years ago
|
||
mozreview-review |
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 26•7 years ago
|
||
mozreview-review |
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 27•7 years ago
|
||
mozreview-review |
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 28•7 years ago
|
||
mozreview-review |
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 29•7 years ago
|
||
mozreview-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 30•7 years ago
|
||
mozreview-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 31•7 years ago
|
||
mozreview-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 32•7 years ago
|
||
mozreview-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 33•7 years ago
|
||
mozreview-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 34•7 years ago
|
||
mozreview-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 35•7 years ago
|
||
mozreview-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 36•7 years ago
|
||
mozreview-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 37•7 years ago
|
||
mozreview-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 38•7 years ago
|
||
mozreview-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+
Comment 39•7 years ago
|
||
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 :)
Assignee | ||
Comment 40•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 52•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Comment 53•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b99f2166a10b
https://hg.mozilla.org/mozilla-central/rev/af243ffe2e85
https://hg.mozilla.org/mozilla-central/rev/51b62e61e969
https://hg.mozilla.org/mozilla-central/rev/19ee2c8eb9a4
https://hg.mozilla.org/mozilla-central/rev/b3fdcf1aabe3
https://hg.mozilla.org/mozilla-central/rev/ffa3422ee9e2
https://hg.mozilla.org/mozilla-central/rev/7d482ab2b5b8
https://hg.mozilla.org/mozilla-central/rev/31a366f730ab
https://hg.mozilla.org/mozilla-central/rev/cf5aea056d7e
https://hg.mozilla.org/mozilla-central/rev/c074ed11b5dc
https://hg.mozilla.org/mozilla-central/rev/749e2fa440ac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 54•7 years ago
|
||
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
Assignee | ||
Comment 55•7 years ago
|
||
I think it's very unlikely that the PerfHasRTLCharsJA performance improvement is related to the patches from this bug.
Updated•7 years ago
|
status-firefox58:
--- → unaffected
status-firefox59:
--- → unaffected
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•