Closed Bug 863018 Opened 7 years ago Closed 7 years ago
Monkey: Add JSShort String path to Concat Strings / LConcat (fix Dromaeo dom-attr regression)
Bug 861251 removed it but we should add it back to avoid regressing dromaeo dom-attr. See bug 861251 comment 12.
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)
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)
Attachment #738954 - Flags: review?(bhackett1024) → review+
Nominating for tracking-firefox23 because this should fix a Dromaeo benchmark regression from bug 861251.
Summary: IonMonkey: Add JSShortString path to ConcatStrings / LConcat → IonMonkey: Add JSShortString path to ConcatStrings / LConcat (fix Dromaeo dom-attr regression)
Luke, review ping. I'd really like to get this in for 23 to avoid regressing Dromaeo.
Oops, I totally missed the review request. Sorry!
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+
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.
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
Retriggers confirm quite conclusively that this push did indeed cause the leaks.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.