Closed Bug 1073700 Opened 10 years ago Closed 10 years ago

Consider moving getter/setter data out of BaseShape

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

bz just noticed that for CSS2Properties.prototype we have 0.03 MB of BaseShapes and 0.02 MB of shapes.

Right now BaseShape stores the getter/setter info, so each accessor property gets its own base shape. BaseShape has a lot of other fields, most of them redundant for objects with many accessors: clasp, parent, metadata, compartment, &c.

It might be worth adding a new class, AccessorShape, that inherits from Shape and stores the getter/setter bits. Then we no longer need separate BaseShapes for those, their shapes just become a bit fatter.

I just instrumented a browser and about 60% of BaseShapes are for accessor properties.

This scheme could end up being less efficient if there are many shapes with the same getter/setter/clasp/compartment, as we will no longer share the getter/setter part and have slightly fatter shapes. I'll try to get some data on this.
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
I have some WIP patches for this. Below are some measurements on 32-bit OS X.

For the notorious CSS2PropertiesPrototype object mentioned in comment 0:

Before:

─33,752 B (00.02%) -- class(CSS2PropertiesPrototype)
 ├──33,672 B (00.02%) -- shapes
 │  ├──29,544 B (00.02%) -- gc-heap
 │  │  ├──18,480 B (00.01%) ── base
 │  │  └──11,064 B (00.01%) ── tree
 │  └───4,128 B (00.00%) ── malloc-heap/tree-tables

After:

─18,976 B (00.01%) -- class(CSS2PropertiesPrototype)
 ├──18,896 B (00.01%) -- shapes
 │  ├──14,768 B (00.01%) -- gc-heap
 │  │  ├──14,672 B (00.01%) ── tree
 │  │  └──────96 B (00.00%) ── base
 │  └───4,128 B (00.00%) ── malloc-heap/tree-tables

The number of BaseShapes goes from 462 -> 3. Although the shapes become a bit bigger, the gc-heap size is more than halved.

Some browser numbers (after opening gmail.com, a Facebook page and bing.com/maps in different tabs and waiting 15 seconds):

Before:

shapes gc-heap: 10.02 MB, 10.25 MB, 10.25 MB
  tree        :  5.59 MB,  5.67 MB,  5.66 MB
  base        :  2.86 MB,  2.96 MB,  2.97 MB
  dict        :  1.58 MB,  1.62 MB,  1.62 MB

After:

shapes gc-heap:  8.87 MB,  8.88 MB,  8.80 MB
  tree        :  5.88 MB,  5.92 MB,  5.85 MB
  base        :  1.30 MB,  1.29 MB,  1.28 MB
  dict        :  1.69 MB,  1.68 MB,  1.67 MB

compartment-tables before: 2.49 MB, 2.53 MB, 2.51 MB
compartment-tables after : 1.82 MB, 1.83 MB, 1.74 MB

unused-gc-things before: 13.22 MB, 12.93 MB, 12.88 MB
unused-gc-things after : 11.90 MB, 13.12 MB, 12.26 MB

So we eliminated most BaseShapes and the compartment tables are also a lot smaller.
Attached patch Patch (obsolete) — Splinter Review
This patch works as described in comment 0. See comment 1 for the results.

Initially I tried moving the getter/setter methods from Shape to AccessorShape, but this got pretty awkward and verbose. The current patch keeps them as Shape methods, so that the Shape/AccessorShape split is an implementation detail and consumers don't have to know about this.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8500531 - Flags: review?(bhackett1024)
Comment on attachment 8500531 [details] [diff] [review]
Patch

Hrm, Try pointed out an issue with this patch (that I should have realized before):

StackShape::hash() now includes the getter + setter. If one of those is an object allocated in the nursery and is moved on GC, we need to rekey all KidsHash tables that contain this shape.

Unfortunately I don't really have a good fix for this. If we don't include the getter/setter in the hash, there will be a lot more collisions I think. Will investigate more tomorrow.
Attachment #8500531 - Flags: review?(bhackett1024)
Attached patch PatchSplinter Review
This version fixes the Generational GC issue in comment 3, by using a custom post barrier that updates the shape's hash when the getter/setter moves. Try is green now.

The patch also makes some changes for Compacting GC (now passes jit-tests with CGC as well).
Attachment #8500531 - Attachment is obsolete: true
Attachment #8501596 - Flags: review?(bhackett1024)
Comment on attachment 8501596 [details] [diff] [review]
Patch

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

Nice!

::: js/src/vm/Shape.cpp
@@ +625,5 @@
>          uint32_t index;
>          bool indexed = js_IdIsIndex(id, &index);
>  
>          Rooted<UnownedBaseShape*> nbase(cx);
> +        if (last->matchesGetterSetter(getter, setter) && !indexed) {

Since the BaseShape doesn't include getter/setter information at all now, I don't think the matchesGetterSetter part of this test is necessary anymore.

::: js/src/vm/Shape.h
@@ +771,5 @@
>           * hint for type inference.
>           */
>          OVERWRITTEN     = 0x04,
>  
> +        ACCESSOR_SHAPE  = 0x08,

Needs a comment.
Attachment #8501596 - Flags: review?(bhackett1024) → review+
For posterity, on AWSY, "JS: After TP5 [+30s]" (Miscellaneous measurements graph) dropped from 99 MB to 92-93 MB.

Also some wins on the other graphs, but they are more noisy.
> For posterity, on AWSY, "JS: After TP5 [+30s]" (Miscellaneous measurements
> graph) dropped from 99 MB to 92-93 MB.

Excellent. It's not easy to get a clear downward signal like that on AWSY :)
https://hg.mozilla.org/mozilla-central/rev/48ef078126bf
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Could this change be responsible for build bustage reported in Bug 1081523?
(In reply to Stefan Sitter from comment #10)
> Could this change be responsible for build bustage reported in Bug 1081523?

Yes, --disable-gcgenerational bustage, sorry. Posted a patch in that bug.
Depends on: 1082662
You need to log in before you can comment on or make changes to this bug.