Compact arenas containing strings

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript: GC
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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

a year ago
Created attachment 8733440 [details] [diff] [review]
compact-strings

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

a year 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 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.
(Assignee)

Comment 5

a year 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.
(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

a year 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 8

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/36c1fd35d995

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/36c1fd35d995
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1259490
Blocks: 1260198
Depends on: 1260371
No longer blocks: 1260198
Depends on: 1260198
erahm: did this make a noticeable difference on AWSY?
Depends on: 1260610
You need to log in before you can comment on or make changes to this bug.