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.

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.

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 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 {}

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

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?

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.

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

Pushed this again, with bug 1119081: https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f042eed356

Excellent! Thank you, jandem.