Closed Bug 1258453 Opened 4 years ago Closed 4 years ago

Compact arenas containing strings

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attached patch compact-stringsSplinter Review
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)
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 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+
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.
(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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/36c1fd35d995
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1260198
Depends on: 1260371
No longer blocks: 1260198
Depends on: 1260198
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.