Use JSInlineString where possible in ConcatStrings()

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8529977 [details] [diff] [review]
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.
Attachment #8529977 - Flags: review?(jdemooij)

Updated

4 years ago
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)
(Assignee)

Comment 4

4 years ago
Created attachment 8544377 [details] [diff] [review]
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?
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+
(Assignee)

Comment 7

4 years ago
I filed bug 1119081 for the JIT code follow-up.
Depends on: 1119180
https://hg.mozilla.org/mozilla-central/rev/51e4e9fcde24
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Assignee)

Comment 9

4 years ago
Backed out for causing some Dromaeo and Octane regressions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7877a00d7025
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Comment 11

4 years ago
(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.
(Assignee)

Comment 12

4 years ago
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.
(Assignee)

Comment 14

4 years ago
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.
(Assignee)

Comment 16

4 years ago
> 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)
Created attachment 8546673 [details] [diff] [review]
Rebased

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+
(Assignee)

Comment 20

4 years ago
Excellent! Thank you, jandem.
https://hg.mozilla.org/mozilla-central/rev/a5f042eed356
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.