1372182 - Optimize adding/defining properties more

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

There's still some overhead here that shows up a lot in profiles because things like |o[x] = y| are hard to optimize with IC stubs.

I have a stack of patches that improves the micro-benchmark below from ~1000 ms to ~774 ms, so about a 25% improvement.

function f() {
    var t = new Date;
    for (var i = 0; i < 100000; i++) {
	var o = {};
	for (var j = 0; j < 40; j++)
	    o["foo" + j] = i;
    }
    print(new Date - t);
}
f();

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Inline NativeObject::dynamicSlotsCount

This ensures dynamicSlotsCount and numDynamicSlots are inlined.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Optimize NativeObject::allocSlot

allocSlot is a bit complicated for dictionary objects, but for non-dictionary objects it just uses the next slot.

This patch renames allocSlot to allocDictionarySlot and uses it only for dictionary objects. There was just one place where we called allocSlot on non-dictionary objects and we can just inline the non-dictionary code there.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Merge jspropertytree.* with vm/Shape*

It's not very clear what belongs in jspropertytree.* or in vm/Shape.*. Having some Shape methods in jspropertytree.cpp and others in vm/Shape.cpp is pretty confusing. Multiple files also makes it harder to optimize this, both manually and for the compiler.

So this patch moves everything in jspropertytree.* to vm/Shape.*

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Some PropertyTree::getChild changes

This restructures PropertyTree::getChild a bit.

IsAboutToBeFinalizedUnbarriered is not inlined, so I added a zone->isGCSweepingOrCompacting() check before calling that.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Inline PropertyTree::getChild in NativeObject::getChildProperty

This adds PropertyTree::inlinedGetChild so we can ensure it is inlined into NativeObject::getChildProperty.

Another approach would be to move PropertyTree::getChild to Shape-inl.h. That works but getChild is pretty big and most callers of getChild probably don't need the inlining.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Inline PurgeEnvironmentChain

Clang was not inlining PurgeEnvironmentChain. Pretty sad because the isDelegate() check means we usually return immediately but still have the call overhead etc.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Inline more code

NativeObject::addProperty does very little work, it basically just forwards to addPropertyInternal. So we might as well inline addProperty into its callers so we only have the addPropertyInternal call.

I also noticed Clang not inlining HeapSlot::set and some other methods, so this adds more MOZ_ALWAYS_INLINE.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Optimize AddTypePropertyId

AddTypePropertyId is pretty slow. This patch adds a fast path for when the property type already exists.

It's a pretty big win (IIRC about 100 ms on the micro-benchmark) because AddTypePropertyId is much slower.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 9 - Minor changes

The last one.

Comment 10

[André Bargull [:anba]]

Comment on attachment 8876705 [details] [diff] [review]
Part 2 - Optimize NativeObject::allocSlot

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

::: js/src/vm/NativeObject.cpp
@@ +1086,5 @@
>  
> +    // Try to pull a free slot from the shape table's slot-number free list.
> +    // Shapes without a ShapeTable have an empty free list, because we only
> +    // purge ShapeTables with an empty free list.
> +    AutoCheckCannotGC nogc;

I'm not sure this |AutoCheckCannotGC| can be moved out of the block, let's see if rooting analysis reports any errors.

Comment 11

[André Bargull [:anba]]

Comment on attachment 8876726 [details] [diff] [review]
Part 7 - Inline more code

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

A bit sad to see we need to force inlining so many methods.

Comment 12

[André Bargull [:anba]]

Comment on attachment 8876741 [details] [diff] [review]
Part 9 - Minor changes

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

LGTM except for the potential dense elements lookup regression.

::: js/src/vm/NativeObject-inl.h
@@ +722,5 @@
> +    if (Shape* shape = obj->lastProperty()->search(cx, id)) {
> +        propp.setNativeProperty(shape);
> +        *donep = true;
> +        return true;
> +    }

I'm not sure about this change. Won't this hurt performance for dense element lookups if we first perform a shape search?

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8876707 [details] [diff] [review]
Part 3 - Merge jspropertytree.* with vm/Shape*

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

Oh wild, we are actually defining Shape methods in that file. And eliminating js*.cpp files is always a plus!

Comment 14

[Tom S [:evilpie]]

Comment on attachment 8876714 [details] [diff] [review]
Part 5 - Inline PropertyTree::getChild in NativeObject::getChildProperty

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

::: js/src/vm/Shape.cpp
@@ +1388,1 @@
>  ShapeHasher::hash(const Lookup& l)

Interesting, you would assume a function this size would always be inlined already.

Comment 15

[Tom S [:evilpie]]

Comment on attachment 8876739 [details] [diff] [review]
Part 8 - Optimize AddTypePropertyId

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

Makes sense I think. This basically duplicates the first few lines of AddTypePropertyId, without the singleton condition.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d64ea16d84e
part 1 - Inline NativeObject::dynamicSlotsCount and related methods. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b796cbce14
part 2 - Refactor NativeObject::allocSlot to make things faster for non-dictionary objects. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e9c71b3bf2
part 3 - Merge jspropertytree.* with vm/Shape.* r=evilpie

Comment 17

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #10)
> I'm not sure this |AutoCheckCannotGC| can be moved out of the block, let's
> see if rooting analysis reports any errors.

Good catch. I restored the {}, to not make the no-gc scope bigger than necessary.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3d64ea16d84e
https://hg.mozilla.org/mozilla-central/rev/39b796cbce14
https://hg.mozilla.org/mozilla-central/rev/66e9c71b3bf2

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7fdcc27f9d9
part 4 - Refactor/optimize PropertyTree::getChild a bit. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/64cde533fc27
part 5 - Inline PropertyTree::getChild in NativeObject::getChildProperty. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/d12ee58d7b27
part 6 - Inline PurgeEnvironmentChain. r=anba

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f7fdcc27f9d9
https://hg.mozilla.org/mozilla-central/rev/64cde533fc27
https://hg.mozilla.org/mozilla-central/rev/d12ee58d7b27

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e689cd66393c
part 7 - Inline some more functions. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cae295906a1
part 8 - Optimize AddTypePropertyId. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/c277ca7f0824
part 9 - Inline more functions. r=anba

Comment 22

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #12)
> I'm not sure about this change. Won't this hurt performance for dense
> element lookups if we first perform a shape search?

Fair enough, I changed the order again.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e689cd66393c
https://hg.mozilla.org/mozilla-central/rev/9cae295906a1
https://hg.mozilla.org/mozilla-central/rev/c277ca7f0824

Comment 24

[Ionuț Goldan [:igoldan]]

== Change summary for alert #7400 (as of June 19 2017 12:00 UTC) ==

Improvements:

  3%  dromaeo_css summary windows7-32 opt e10s     6,666.74 -> 6,894.96

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7400