Use JSInlineString where possible in ConcatStrings()

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
ConcatStringsInline() is an ugly function, but I couldn't see a better way to
avoid explicitly duplicating the code.
Attachment #8529977 - Flags: review?(jdemooij)
Duplicate of this bug: 1038095
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.
Attachment #8529977 - Flags: review?(jdemooij)
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?
Attachment #8544377 - Flags: review?(jdemooij)
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 {}
Attachment #8544377 - Flags: review?(jdemooij) → review+
I filed bug 1119081 for the JIT code follow-up.
Depends on: 1119180
https://hg.mozilla.org/mozilla-central/rev/51e4e9fcde24
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Backed out for causing some Dromaeo and Octane regressions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7877a00d7025
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.
(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.
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?
Flags: needinfo?(jdemooij)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Unless I'm mistaken, the only effect the patch has is to create some JSInlineStrings where previously we created JSFatInlineStrings.
(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.
> 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...
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.
Flags: needinfo?(jdemooij)
Posted patch RebasedSplinter Review
The PJS removal severely bitrotted this patch (no more ThreadSafeContext and ScopedThreadSafeStringInspector), here's a rebased version.
Attachment #8529977 - Attachment is obsolete: true
Attachment #8546673 - Flags: review+
Excellent! Thank you, jandem.
https://hg.mozilla.org/mozilla-central/rev/a5f042eed356
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.