1039551 - NewString/NewStringDontDeflate should create inline strings if possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Unlike NewStringCopyN, NewString does not attempt to allocate an inline string. I just instrumented the browser and in many cases the length is short enough that we really want an inline string.

NewString* is not as hot as NewStringCopy*, but I think it's good to be consistent and to use inline strings as much as possible, to save memory and avoid unnecessary fragmentation.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8456919 [details] [diff] [review]
Patch

Review of attachment 8456919 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsstr.cpp
@@ +4399,5 @@
>              return cx->staticStrings().getUnit(c);
>          }
>      }
>  
> +    if (JSFatInlineString::lengthFits<CharT>(length) && EnableLatin1Strings) {

Hm I thought checking EnableLatin1Strings was necessary (because NewFatInlineString attemps to inflate if latin1 is disabled) but it probably isn't (if EnableLatin1Strings == false, CharT should be != Latin1Char or else the JSFlatString::new_ below would be wrong, so I'll remove it.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8456919 [details] [diff] [review]
Patch

Review of attachment 8456919 [details] [diff] [review]:
-----------------------------------------------------------------

Is it worth checking to see if it fits within a non-fat inline string first?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Is it worth checking to see if it fits within a non-fat inline string first?

NewFatInlineString calls AllocateFatInlineString, which will try to use a non-fat inline string if possible.

We should really rename NewFatInlineString to NewInlineString, NewInlineOrFatInlineString or something...

Comment 4

[Nicholas Nethercote [inactive]]

> We should really rename NewFatInlineString to NewInlineString,
> NewInlineOrFatInlineString or something...

I wonder if it's worth renaming "JSInlineString" as "JSSlimInlineString", which would allow us to use "inline" to refer to both kinds.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> I wonder if it's worth renaming "JSInlineString" as "JSSlimInlineString",
> which would allow us to use "inline" to refer to both kinds.

Yeah, but JSFatInlineString inherits from JSInlineString and we often use JSInlineString* for both kinds. Changing it to JSSlimInlineString* would also be confusing there.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4db87a48b24

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows debug mochitest-4 crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a41861d55096

https://tbpl.mozilla.org/php/getParsedLog.php?id=44119167&tree=Mozilla-Inbound

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #7)
> Backed out for Windows debug mochitest-4 crashes.

Hm, sorry for that. I doubt it's directly caused by this patch though; it's pretty simple and it works fine everywhere else. Maybe it exposed another issue somewhere, will investigate.

Comment 10

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3fc15a1659

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb3fc15a1659