Optimize adding/defining properties more

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(9 attachments)

Assignee

Description

2 years ago
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();
Assignee

Comment 1

2 years ago
This ensures dynamicSlotsCount and numDynamicSlots are inlined.
Attachment #8876704 - Flags: review?(andrebargull)
Assignee

Comment 2

2 years ago
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.
Attachment #8876705 - Flags: review?(andrebargull)
Assignee

Comment 3

2 years ago
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.*
Attachment #8876707 - Flags: review?(evilpies)
Assignee

Comment 4

2 years ago
This restructures PropertyTree::getChild a bit.

IsAboutToBeFinalizedUnbarriered is not inlined, so I added a zone->isGCSweepingOrCompacting() check before calling that.
Attachment #8876712 - Flags: review?(jcoppeard)
Assignee

Comment 5

2 years ago
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.
Attachment #8876714 - Flags: review?(evilpies)
Assignee

Comment 6

2 years ago
Clang was not inlining PurgeEnvironmentChain. Pretty sad because the isDelegate() check means we usually return immediately but still have the call overhead etc.
Attachment #8876715 - Flags: review?(andrebargull)
Attachment #8876712 - Flags: review?(jcoppeard) → review+
Assignee

Comment 7

2 years ago
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.
Attachment #8876726 - Flags: review?(andrebargull)
Assignee

Comment 8

2 years ago
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.
Attachment #8876739 - Flags: review?(evilpies)
Assignee

Comment 9

2 years ago
The last one.
Attachment #8876741 - Flags: review?(andrebargull)
Attachment #8876704 - Flags: review?(andrebargull) → review+
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.
Attachment #8876705 - Flags: review?(andrebargull) → review+
Attachment #8876715 - Flags: review?(andrebargull) → review+
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.
Attachment #8876726 - Flags: review?(andrebargull) → review+
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?
Attachment #8876741 - Flags: review?(andrebargull) → review+
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!
Attachment #8876707 - Flags: review?(evilpies) → review+
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.
Attachment #8876714 - Flags: review?(evilpies) → review+
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.
Attachment #8876739 - Flags: review?(evilpies) → review+

Comment 16

2 years ago
Pushed by jandemooij@gmail.com:
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
Assignee

Comment 17

2 years ago
(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.
Keywords: leave-open

Comment 19

2 years ago
Pushed by jandemooij@gmail.com:
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 21

2 years ago
Pushed by jandemooij@gmail.com:
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
Assignee

Comment 22

2 years ago
(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.
Keywords: leave-open

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e689cd66393c
https://hg.mozilla.org/mozilla-central/rev/9cae295906a1
https://hg.mozilla.org/mozilla-central/rev/c277ca7f0824
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
== 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
You need to log in before you can comment on or make changes to this bug.