1119081 - Use JSInlineString where possible when concatenating strings in JITted code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 1105895 changed ConcatString() to use a non-fat inline string where possible. We should do the same for JITted concatenation code. Jan said:

> 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 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This matches what njn is doing for the ConcatStrings VM function in bug 1105895 and JIT code should behave the same to avoid benchmark regressions (see bug 1105895 comment 15 for the details).

Comment 2

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75b65a499a57

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/75b65a499a57

Comment 4

[Hannes Verschore [:h4writer]]

@Jan: I didn't look into what changes are happening, but do we need a similar change for "visitSubstr"? Since we create strings in assembly in LSubstr too? I think not, but just asking to be sure we don't miss that.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #4)
> @Jan: I didn't look into what changes are happening, but do we need a
> similar change for "visitSubstr"? Since we create strings in assembly in
> LSubstr too? I think not, but just asking to be sure we don't miss that.

Good point. The code we emit in visitSubstr always creates a FatInlineString in the inline case. JSDependentString::new_ creates a fat inline or non-fat inline string. So it seems the JIT code there needs the same fix.

If we have LSubstr (with a very short result string) in a loop, the current code should be measurably slower. Do you want to file/fix?

Btw, the code in CreateDependentString does handle this, could we use that somehow?