Closed
Bug 1258453
Opened 8 years ago
Closed 8 years ago
Compact arenas containing strings
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
20.46 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
Following on from bug 1257903, we can also compact arenas containing strings. I originally thought this would require browser changes but then I realised that the browser atomizes strings whose address it expects to stay constant, and we don't compact the atoms zone.
Assignee | ||
Comment 1•8 years ago
|
||
Patch to compact strings. Two wrinkles: AutoStableStringChars needs to copy the chars if the string data is stored inline, since it may be moved. I don't think this will be a performance problem but if it is we can allocate enough space in the ASSC structure to copy it into to get rid of the allocation. Also I changed the way NativeIterator initialisation works so that the property strings are set on a NativeIterator that belongs to a rooted PropertyIteratorObject. This is slightly hacky but ensures everything gets traced correctly if we GC during initialisation, and it does away with putting the strings in to an auxiliary vector just for this purpose which doesn't work if the strings get moved.
Attachment #8733440 -
Flags: review?(terrence)
Assignee | ||
Comment 2•8 years ago
|
||
Some numbers from about:memory before and after compacting showing unused space in string arenas reduced from 15.75MB -> 1.26MB for a 220MB heap. Before: 293.85 MB (100.0%) -- js-main-runtime-gc-heap-committed ├──223.79 MB (76.16%) -- used │ ├──213.89 MB (72.79%) -- gc-things │ │ ├──107.30 MB (36.51%) ── objects │ │ ├───45.80 MB (15.59%) ── shapes │ │ ├───17.73 MB (06.04%) ── scripts │ │ ├───15.06 MB (05.12%) ── strings │ │ ├───12.61 MB (04.29%) ── object-groups │ │ ├───12.58 MB (04.28%) ── lazy-scripts │ │ └────2.82 MB (00.96%) -- (3 tiny) │ │ ├──2.74 MB (00.93%) ── base-shapes │ │ ├──0.08 MB (00.03%) ── jitcode │ │ └──0.00 MB (00.00%) ── symbols │ ├────5.11 MB (01.74%) ── chunk-admin │ └────4.79 MB (01.63%) ── arena-admin └───70.06 MB (23.84%) -- unused ├──69.06 MB (23.50%) -- gc-things │ ├──33.68 MB (11.46%) ── objects │ ├──15.75 MB (05.36%) ── strings │ ├───8.16 MB (02.78%) ── shapes │ ├───5.11 MB (01.74%) ── scripts │ ├───3.85 MB (01.31%) ── object-groups │ └───2.51 MB (00.85%) -- (4 tiny) │ ├──1.46 MB (00.50%) ── lazy-scripts │ ├──0.92 MB (00.31%) ── base-shapes │ ├──0.12 MB (00.04%) ── jitcode │ └──0.01 MB (00.00%) ── symbols └───1.00 MB (00.34%) -- (2 tiny) ├──1.00 MB (00.34%) ── chunks └──0.00 MB (00.00%) ── arenas After: 237.94 MB (100.0%) -- js-main-runtime-gc-heap-committed ├──220.68 MB (92.75%) -- used │ ├──211.61 MB (88.94%) -- gc-things │ │ ├──107.33 MB (45.11%) ── objects │ │ ├───45.86 MB (19.28%) ── shapes │ │ ├───15.21 MB (06.39%) ── scripts │ │ ├───15.12 MB (06.36%) ── strings │ │ ├───12.63 MB (05.31%) ── object-groups │ │ ├───12.58 MB (05.29%) ── lazy-scripts │ │ ├────2.75 MB (01.15%) ── base-shapes │ │ └────0.13 MB (00.06%) ++ (2 tiny) │ ├────5.08 MB (02.13%) ── chunk-admin │ └────3.99 MB (01.68%) ── arena-admin └───17.26 MB (07.25%) -- unused ├──16.26 MB (06.83%) -- gc-things │ ├───6.88 MB (02.89%) ── scripts │ ├───5.55 MB (02.33%) -- (7 tiny) │ │ ├──1.57 MB (00.66%) ── objects │ │ ├──1.46 MB (00.61%) ── lazy-scripts │ │ ├──1.26 MB (00.53%) ── strings │ │ ├──0.91 MB (00.38%) ── base-shapes │ │ ├──0.22 MB (00.09%) ── shapes │ │ ├──0.12 MB (00.05%) ── jitcode │ │ └──0.01 MB (00.00%) ── symbols │ └───3.83 MB (01.61%) ── object-groups └───1.00 MB (00.42%) -- (2 tiny) ├──1.00 MB (00.42%) ── chunks └──0.00 MB (00.00%) ── arenas
Comment 3•8 years ago
|
||
Comment on attachment 8733440 [details] [diff] [review] compact-strings Review of attachment 8733440 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/jsgc.cpp @@ +2553,5 @@ > { > MOZ_ASSERT(IsValidAllocKind(kind)); > > + // GC things that do not contain pointers to cells we relocate don't need > + // updating. Note that AllocKind::STRING is the only string kind which can Extra space. ::: js/src/jsiter.cpp @@ +57,4 @@ > { > + for (HeapPtrFlatString* str = begin(); str < end(); str++) { > + if (*str) > + TraceEdge(trc, str, "NativeIterator property"); TraceNullableEdge if that goes in first. @@ +63,2 @@ > if (obj) > + TraceEdge(trc, &obj, "NativeIterator::obj"); Actually, I think this was in your TraceNullableEdge patch? ::: js/src/vm/String.cpp @@ +908,5 @@ > state_ = TwoByte; > twoByteChars_ = linearString->rawTwoByteChars(); > } > > + Stray newline?
Attachment #8733440 -
Flags: review?(terrence) → review+
Comment 4•8 years ago
|
||
Great numbers! (In reply to Jon Coppeard (:jonco) from comment #1) > AutoStableStringChars needs to copy the chars if the string data is stored > inline, since it may be moved. I don't think this will be a performance > problem but if it is we can allocate enough space in the ASSC structure to > copy it into to get rid of the allocation. Can we add a Vector to ASSC and use .resize() instead of malloc/free? That should let us get rid of small malloc/free without additional complexity.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #4) > Can we add a Vector to ASSC and use .resize() instead of malloc/free? That > should let us get rid of small malloc/free without additional complexity. This is a great idea. One issue though is how to implement maybeGiveOwnershipToCaller() - Vector has extractRawBuffer() but this will allocate and copy if the vector is using its inline space.
Comment 6•8 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #5) > One issue though is how to implement > maybeGiveOwnershipToCaller() - Vector has extractRawBuffer() but this will > allocate and copy if the vector is using its inline space. Ah, fair enough. I don't think ASSC is used in super-hot code, so maybe best to stick with plain malloc.
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #6) > Ah, fair enough. I don't think ASSC is used in super-hot code, so maybe best > to stick with plain malloc. Ok, I've filed bug 1259021 to look into it anyway.
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/36c1fd35d995
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Comment 10•8 years ago
|
||
erahm: did this make a noticeable difference on AWSY?
You need to log in
before you can comment on or make changes to this bug.
Description
•