1105895 - Use JSInlineString where possible in ConcatStrings()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

In ConcatStrings() we create a JSFatInlineString if the result is short enough,
but we never try to create an JSInlineString, like we do elsewhere (e.g. in
NewFatInlineString()). We should.

Here are some measurements. I checked if each ConcatStrings result would fit
into an Inline string, a FatInline string, or neither, for both 32-bit and
64-bit. The workload I used was: start the browser and open gmail.com,
nytimes.com, and cnn.com, then quit.

64-bit results:

> 92888 counts:
> (  1)    38844 (41.8%, 41.8%): 64L: Inline
> (  2)    28631 (30.8%, 72.6%): 64L: Neither
> (  3)    13092 (14.1%, 86.7%): 64L: FatInline
> (  4)    11734 (12.6%, 99.4%): 64T: Neither
> (  5)      452 ( 0.5%, 99.9%): 64T: Inline
> (  6)      112 ( 0.1%,100.0%): 64T: FatInline

(The 'L' suffix means the string was latin1; the 'T' suffix means it was
twoByte.)

41.8 + 0.5 = 42.3% of strings could be Inline, but we're making them FatInline.

32-bit results:

> 92908 counts:
> (  1)    34261 (36.9%, 36.9%): 32L: FatInline
> (  2)    28637 (30.8%, 67.7%): 32L: Neither
> (  3)    17689 (19.0%, 86.7%): 32L: Inline
> (  4)    11735 (12.6%, 99.4%): 32T: Neither
> (  5)      411 ( 0.4%, 99.8%): 32T: FatInline
> (  6)      153 ( 0.2%,100.0%): 32T: Inline

19.0 + 0.2 = 19.2% of strings could be Inline, but we're making them FatInline.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Use JSInlineString where possible in ConcatStrings()

ConcatStringsInline() is an ugly function, but I couldn't see a better way to
avoid explicitly duplicating the code.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8529977 [details] [diff] [review]
Use JSInlineString where possible in ConcatStrings()

Review of attachment 8529977 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/String.cpp
@@ +521,5 @@
>          return nullptr;
>  
>      bool isLatin1 = left->hasLatin1Chars() && right->hasLatin1Chars();
> +
> +    bool canUseInline = isLatin1

I think this could be simplified by using AllocateFatInlineString in String-inl.h The name is a bit confusing (we should probably rename it..) but it allocates an inline or a fat inline string and also returns you the char pointer.

Also, the JIT inlines this code in JitCompartment::generateStringConcatStub, we should probably do the same thing there to be consistent. It seems like you can do this in ConcatFatInlineString, the concatenated length is in temp2 at the start of that function, so we should be able to branch on that.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Use JSInlineString where possible in ConcatStrings()

Sorry for the delay in getting back to this.

> I think this could be simplified by using AllocateFatInlineString in
> String-inl.h The name is a bit confusing (we should probably rename it..) but
> it allocates an inline or a fat inline string and also returns you the char
> pointer.

I've done that, it's much better.

> Also, the JIT inlines this code in JitCompartment::generateStringConcatStub,
> we should probably do the same thing there to be consistent. It seems like you
> can do this in ConcatFatInlineString, the concatenated length is in temp2 at
> the start of that function, so we should be able to branch on that.

I haven't done that, and it looks painful (for me, at least). Ok to leave as a
follow-up, do you think?

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8544377 [details] [diff] [review]
Use JSInlineString where possible in ConcatStrings()

Review of attachment 8544377 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

(In reply to Nicholas Nethercote [:njn] from comment #4)
> I haven't done that, and it looks painful (for me, at least). Ok to leave as
> a follow-up, do you think?

OK, that's fine.

::: js/src/vm/String.cpp
@@ +493,5 @@
> +        if (isLatin1) {
> +            str = AllocateFatInlineString<allowGC>(cx, wholeLength, &latin1Buf);
> +        } else {
> +            str = AllocateFatInlineString<allowGC>(cx, wholeLength, &twoByteBuf);
> +        }

Nit: no {}

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51e4e9fcde24

Comment 7

[Nicholas Nethercote [inactive]]

I filed bug 1119081 for the JIT code follow-up.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51e4e9fcde24

Comment 9

[Nicholas Nethercote [inactive]]

Backed out for causing some Dromaeo and Octane regressions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7877a00d7025

Comment 10

[Nicholas Nethercote [inactive]]

I can reproduce the deltablue regression locally. Somehow with the patch applied ConcatStrings ends up being called 45,000 times instead of 3,000. No idea why yet.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> I can reproduce the deltablue regression locally. Somehow with the patch
> applied ConcatStrings ends up being called 45,000 times instead of 3,000. No
> idea why yet.

It's something to do with Ion. When I run with --no-ion the difference goes away and ConcatStrings() is called 45,000 times both with and without the patch. Huh.

Comment 12

[Nicholas Nethercote [inactive]]

So there must be some place in Ion where it JITs if it gets a JSFatInlineString but bails if it gets a JSInlineString... or something like that. But I couldn't find it. jandem, any ideas?

Comment 13

[Boris Zbarsky [:bzbarsky]]

If nothing else, the jitcode for concat will bail through to the vmcall if it has a rope coming in.  See the top of ConcateFatInlineString in CodeGenerator.cpp.  Is it possible we ended up with more ropes coming through there?

Note that the DOM thing that regressed is also the one that concatenated short strings, by the way.

Comment 14

[Nicholas Nethercote [inactive]]

Unless I'm mistaken, the only effect the patch has is to create some JSInlineStrings where previously we created JSFatInlineStrings.

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> jandem, any ideas?

One possibility is this: when the free list is empty, Ion's string concat stub will fall back to the ConcatStrings VM call. This VM call then is supposed to do the allocation and refill the free list, so that the next X allocations from JIT code will succeed.

So it's possible that with the patch, the JIT code tries to allocate fat-inline strings, then bails to the VM where we allocate a non-fat-inline string and hence don't fill the fat-inline string free list...

I can look into this in a bit, after pushing some patches etc.

Comment 16

[Nicholas Nethercote [inactive]]

> So it's possible that with the patch, the JIT code tries to allocate
> fat-inline strings, then bails to the VM where we allocate a non-fat-inline
> string and hence don't fill the fat-inline string free list...

In which case the JIT follow-up (bug 1119081) would fix this...

Comment 17

[Jan de Mooij [:jandem]]

Yup, comment 15 seems to be right: bug 1119081 fixes the deltablue regression. Performance might even be slightly better than before. Will post the patch in a bit.

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Rebased

The PJS removal severely bitrotted this patch (no more ThreadSafeContext and ScopedThreadSafeStringInspector), here's a rebased version.

Comment 19

[Jan de Mooij [:jandem]]

Pushed this again, with bug 1119081:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f042eed356

Comment 20

[Nicholas Nethercote [inactive]]

Excellent! Thank you, jandem.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a5f042eed356