Closed
Bug 863018
Opened 8 years ago
Closed 8 years ago
IonMonkey: Add JSShortString path to ConcatStrings / LConcat (fix Dromaeo dom-attr regression)
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla23
Tracking | Status | |
---|---|---|
firefox22 | --- | unaffected |
firefox23 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
(Keywords: perf, regression)
Attachments
(2 files)
25.95 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
3.66 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
Bug 861251 removed it but we should add it back to avoid regressing dromaeo dom-attr. See bug 861251 comment 12.
Assignee | ||
Comment 1•8 years ago
|
||
Adds a new IonCode stub to IonCompartment for concatenating strings. The existing concat logic is moved into it, and now supports creating short strings inline. This should turn the dom-attr regression into a small win.
Attachment #738945 -
Flags: review?(luke)
Assignee | ||
Comment 2•8 years ago
|
||
With the previous patch LConcat is the first non-call instruction with a safepoint and useFixed/tempFixed. This causes a regalloc integrity check failure. This patch fixes it, as discussed on IRC.
Attachment #738954 -
Flags: review?(bhackett1024)
Updated•8 years ago
|
Attachment #738954 -
Flags: review?(bhackett1024) → review+
Comment 3•8 years ago
|
||
Nominating for tracking-firefox23 because this should fix a Dromaeo benchmark regression from bug 861251.
Blocks: 861251
status-firefox22:
--- → unaffected
status-firefox23:
--- → affected
tracking-firefox23:
--- → ?
Keywords: perf,
regression
Summary: IonMonkey: Add JSShortString path to ConcatStrings / LConcat → IonMonkey: Add JSShortString path to ConcatStrings / LConcat (fix Dromaeo dom-attr regression)
Assignee | ||
Comment 4•8 years ago
|
||
Luke, review ping. I'd really like to get this in for 23 to avoid regressing Dromaeo.
![]() |
||
Comment 5•8 years ago
|
||
Oops, I totally missed the review request. Sorry!
![]() |
||
Comment 6•8 years ago
|
||
Comment on attachment 738945 [details] [diff] [review] Patch Review of attachment 738945 [details] [diff] [review]: ----------------------------------------------------------------- Nice work! ::: js/src/ion/CodeGenerator.cpp @@ +3555,5 @@ > + masm.bind(&isShort); > + > + // State: lhs length in temp1, result length in temp2. > + > + // Ensure both strings are linear (flags != 0). Could you JS_STATIC_ASSERT(String::ROPE_FLAGS == 0); ? @@ +3576,5 @@ > + > + // Copy lhs chars. Temp1 still holds the lhs length. Note that this > + // advances temp2 to point to the next char. > + masm.loadPtr(Address(lhs, JSString::offsetOfChars()), temp3); > + masm.copyStringChars(temp2, temp3, temp1, temp4); Maybe this doesn't match with pre-existing Ion style, but I think it'd be nicer to have copyStringChars as a static helper right above this, so that I could easily go back and forth to see what gets clobbered, etc. ::: js/src/ion/Ion.cpp @@ +1330,5 @@ > > if (!cx->compartment->ensureIonCompartmentExists(cx)) > return AbortReason_Alloc; > > + if (!cx->compartment->ionCompartment()->ensureIonStubsExist(cx)) It seems like you could put this call inside IonCompartment::initialize to avoid the number of initialization states an IonCompartment can be in? ::: js/src/ion/IonCompartment.h @@ +199,5 @@ > > + // Stub to concatenate two strings inline. Note that it can't be > + // stored in IonRuntime because masm.newGCString bakes in > + // zone-specific pointers. > + ReadBarriered<IonCode> stringConcatStub_; Could you explain why this needs to be read-barriered? IonCompartment::mark always marks this pointer, making it a strong reference, and I thought read barriers were only necessary for weak references.
Attachment #738945 -
Flags: review?(luke) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Pushed with comments addressed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/360788bf2a0f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6604b700492c Linux/Android Try: https://tbpl.mozilla.org/?tree=Try&rev=9edbc16396b2 (In reply to Luke Wagner [:luke] from comment #6) > It seems like you could put this call inside IonCompartment::initialize to > avoid the number of initialization states an IonCompartment can be in? The idea was that IonCompartment is also created for baseline code, and we only need this stub for Ion code, so we only create it when we are Ion-compiling something. But it's only a single stub, so I just added it to initialize for now. > Could you explain why this needs to be read-barriered? IonCompartment::mark > always marks this pointer, making it a strong reference, and I thought read > barriers were only necessary for weak references. All IonCode stubs in IonCompartment used to be ReadBarriered before we moved them to IonRuntime, but you are right, it's not needed here.
Comment 8•8 years ago
|
||
It *appears* that this is responsible for Windows mochitest b-c leaks. Due to the long turnaround time of the tests, I'm backing this out for now to hopefully minimize the inbound closure time. I'll update the bug if and when this is confirmed to be the culprit. https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff756a337e4 https://tbpl.mozilla.org/php/getParsedLog.php?id=22285228&tree=Mozilla-Inbound
Comment 9•8 years ago
|
||
Retriggers confirm quite conclusively that this push did indeed cause the leaks.
Comment 10•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/360788bf2a0f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•8 years ago
|
Assignee | ||
Comment 11•8 years ago
|
||
The problem was that IonCompartment can't hold any strong references or it will keep the whole compartment alive. So I made it a weak pointer, just like the other stubs before they were moved to IonRuntime. For some reason this only failed on Windows, which was not part of my Try push. Looks green now: https://tbpl.mozilla.org/?tree=Try&rev=cb6cd2e1b582 https://hg.mozilla.org/integration/mozilla-inbound/rev/3b7a2cbccaaa
Comment 12•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3b7a2cbccaaa
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•8 years ago
|
Updated•8 years ago
|
tracking-firefox23:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•