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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Bug 861251 removed it but we should add it back to avoid regressing dromaeo dom-attr. See bug 861251 comment 12.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Regalloc fix

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.

Comment 3

[Matt Brubeck (:mbrubeck)]

Nominating for tracking-firefox23 because this should fix a Dromaeo benchmark regression from bug 861251.

Comment 4

[Jan de Mooij [:jandem]]

Luke, review ping. I'd really like to get this in for 23 to avoid regressing Dromaeo.

Comment 5

[Luke Wagner [:luke]]

Oops, I totally missed the review request.  Sorry!

Comment 6

[Luke Wagner [:luke]]

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.

Comment 7

[Jan de Mooij [:jandem]]

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

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

Retriggers confirm quite conclusively that this push did indeed cause the leaks.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/360788bf2a0f

Comment 11

[Jan de Mooij [:jandem]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b7a2cbccaaa