Closed Bug 1119081 Opened 5 years ago Closed 5 years ago

Use JSInlineString where possible when concatenating strings in JITted code

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: njn, Assigned: jandem)

Details

Attachments

(1 file)

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.
Summary: Use JSInlineString where possible in ConcatStrings() → Use JSInlineString where possible when concatenating strings in JITted code
Attached patch PatchSplinter Review
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).
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8546676 - Flags: review?(bhackett1024)
Attachment #8546676 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/75b65a499a57
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
@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.
Flags: needinfo?(jdemooij)
(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?
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.