Closed Bug 1275282 Opened 4 years ago Closed 4 years ago

1.83MB of empty strings on irccloud

Categories

(Core :: JavaScript Engine, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bkelly, Assigned: jandem)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 2 obsolete files)

I just noticed this in my about:memory output for irccloud:

1.83 MB (00.13%) ── string(length=0, copies=79786, "")/gc-heap/latin1

That's 1.83MB of memory for empty strings.  Each string takes at least 24 bytes on 64-bit, so that numbers are correct.  It just seems sub-optimal to make so many copies of the same value.
Whiteboard: [MemShrink]
Note, I'm on FF46.0.1 on windows 10.
Flags: needinfo?(jdemooij)
Stack to where we allocate length-0 strings here (or at least _a_ place):

NewStringDeflated (cx=0x1282a7400, s=0x13ac02e90, n=0)
js::NewStringCopyN<(js::AllowGC)1, char16_t>(cx=0x1282a7400, s=0x13ac02e90, n=0)
js::JSONParser<char16_t>::readString<(js::JSONParserBase::StringType)1>()
Comment on attachment 8756229 [details] [diff] [review]
Patch

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

Nice!
Attachment #8756229 - Flags: review?(luke) → review+
Attached patch Patch (obsolete) — Splinter Review
In some places we checked for length 1 static strings, but not length 0 and length 2. Measurements indicate static strings can be used for most of length 2 strings as well.

The patch cleans this up and checks |length <= 2|.

I also changed StaticStrings::init to call NewInlineString directly, to bypass a bunch of unnecessary checks.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8756225 - Flags: review?(luke)
Attached patch Patch (obsolete) — Splinter Review
The n == 0 checks in JS_NewStringCopyN and JS_NewStringCopyZ are now redundant so we can remove them.
Attachment #8756225 - Attachment is obsolete: true
Attachment #8756225 - Flags: review?(luke)
Attachment #8756228 - Flags: review?(luke)
Attached patch PatchSplinter Review
Attachment #8756228 - Attachment is obsolete: true
Attachment #8756228 - Flags: review?(luke)
Attachment #8756229 - Flags: review?(luke)
https://hg.mozilla.org/mozilla-central/rev/1f5902db90a7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.