Closed Bug 863018 Opened 7 years ago Closed 7 years ago

IonMonkey: Add JSShortString path to ConcatStrings / LConcat (fix Dromaeo dom-attr regression)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- unaffected
firefox23 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Keywords: perf, regression)

Attachments

(2 files)

Bug 861251 removed it but we should add it back to avoid regressing dromaeo dom-attr. See bug 861251 comment 12.
Attached patch PatchSplinter Review
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)
Attached patch Regalloc fixSplinter Review
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.
Blocks: 861251
Keywords: perf, regression
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.
https://hg.mozilla.org/mozilla-central/rev/360788bf2a0f
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: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/3b7a2cbccaaa
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 866611
You need to log in before you can comment on or make changes to this bug.