Closed Bug 991998 Opened 6 years ago Closed 6 years ago

Make short strings have the same max length on 32-bit and 64-bit platforms


(Core :: JavaScript Engine, defect)

Not set





(Reporter: njn, Assigned: njn)



(2 files)

Currently, JSShortString can hold up to 11 chars (plus null terminator) on 32-bit platforms, and up to 23 chars on 64-bit platforms. This is a big difference and can lead to significant performance effects.

For example, ropes don't get used when concatenating if the result is short enough to fit into a JSShortString. If you build up lots of strings one char at a time (as pdf.js used to do when parsing, for example), you get quadratic append behaviour up to that limit, and 23^2 is a lot bigger than 11^2.

I propose reducing the 64-bit max length to 11 to match the 32-bit max. If you look at the sizes involved with GC allocations vs malloc allocations, when you get above 11 chars, most of the time using a JSShortString actually uses more memory than using malloc would, because usually a big chunk of the fat GC thing is unused.

I expect this will make little different to perf or memory usage in most cases.
I've always found the name JSShortString to be confusing... a string of length
10 counts as "short" (because we store it in a JSShortString), but a string of
length 3 doesn't (because we store it in a JSInlineString).

This patch renames JSShortString as JSFatInlineString, which avoids this
problem. I grep'd for all instances of "short" and "Short" and "SHORT", so I am
confident I changed everything that needed changing.
Attachment #8401666 - Flags: review?(luke)
Pretty simple. Here are string measurements when starting up the browser,

│   ├──4.73 MB (12.02%) -- strings
│   │  ├──2.68 MB (06.80%) ── malloc-heap
│   │  └──2.05 MB (05.22%) ── gc-heap

and after:

│   ├──4.62 MB (11.60%) -- strings
│   │  ├──3.18 MB (08.00%) ── malloc-heap
│   │  └──1.44 MB (03.61%) ── gc-heap

As expected -- more malloc-heap, less gc-heap, and slightly less memory overall
due to strings in the length range 12--19 being stored more compactly.  (BTW, I
measured three times and there wasn't much variation.)
Attachment #8401669 - Flags: review?(luke)
Comment on attachment 8401666 [details] [diff] [review]
(part 1) - Rename JSShortString as JSFatInlineString.

Wayyyy better name.
Attachment #8401666 - Flags: review?(luke) → review+
Comment on attachment 8401669 [details] [diff] [review]
(part 2) - Limit JSFatInlineString's length to 11 on both 32-bit and 64-bit platforms.

Attachment #8401669 - Flags: review?(luke) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1008613
No longer depends on: 1008613
You need to log in before you can comment on or make changes to this bug.