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
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, before: │ ├──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. Righteous!
Attachment #8401669 - Flags: review?(luke) → review+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.