Optimize adding/defining new properties on native objects

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:investigate:p1])

Attachments

(9 attachments, 2 obsolete attachments)

1.20 KB, patch
till
: review+
Details | Diff | Splinter Review
1.03 KB, patch
till
: review+
Details | Diff | Splinter Review
3.72 KB, patch
till
: review+
Details | Diff | Splinter Review
4.10 KB, patch
till
: review+
Details | Diff | Splinter Review
4.50 KB, patch
till
: review+
Details | Diff | Splinter Review
4.62 KB, patch
till
: review+
Details | Diff | Splinter Review
6.24 KB, patch
till
: review+
Details | Diff | Splinter Review
1.15 KB, patch
till
: review+
Details | Diff | Splinter Review
935 bytes, patch
till
: review+
Details | Diff | Splinter Review
Assignee

Description

2 years ago
There is a bunch of slowness here that often shows up in profiles.

For instance, SetNonexistentProperty calls SetPropertyByDefining where we then check if the property exists, but SetNonexistentProperty already *knows* the property doesn't exist.

Then via some other calls we end up in AddOrChangeProperty -> NativeObject::putProperty, and there we do *another* lookup!

We can do much better.
Whiteboard: [qf:investigate:p1]
Assignee

Comment 1

2 years ago
NativeObject::addProperty calls IsExtensible, but addProperty is only used on freshly-allocated objects that must be extensible. I replaced it with a MOZ_ASSERT(obj->nonProxyIsExtensible()).
Attachment #8848084 - Flags: review?(till)
Assignee

Comment 2

2 years ago
Like part 1 but for putProperty. The only place where we call putProperty (via AddOrChangeProperty) to add a new property is here:

http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/js/src/vm/NativeObject.cpp#1484

And that code already does an obj->nonProxyIsExtensible() check.
Attachment #8848085 - Flags: review?(till)
Assignee

Comment 3

2 years ago
UpdateShapeTypeAndValue is a very hot function so let's optimize it a bit:

* There are two call sites but according to a profile Clang wasn't inlining it so I added MOZ_ALWAYS_INLINE.

* It always returns true so I changed the return type to void.

* It can't GC so I removed the handle arguments.
Attachment #8848093 - Flags: review?(till)
Assignee

Comment 4

2 years ago
Forgot to qref.
Attachment #8848093 - Attachment is obsolete: true
Attachment #8848093 - Flags: review?(till)
Attachment #8848094 - Flags: review?(till)
Comment on attachment 8848084 [details] [diff] [review]
Part 1 - Remove IsExtensible check from NativeObject::addProperty

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

r=me
Attachment #8848084 - Flags: review?(till) → review+
Comment on attachment 8848085 [details] [diff] [review]
Part 2 - Remove IsExtensible check from NativeObject::putProperty

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

r=me
Attachment #8848085 - Flags: review?(till) → review+
Comment on attachment 8848094 [details] [diff] [review]
Part 3 - Optimize UpdateShapeTypeAndValue

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

Nice, r=me
Attachment #8848094 - Flags: review?(till) → review+

Comment 8

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/565beb14681c
part 1 - Remove IsExtensible check from NativeObject::addProperty. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/062ad3879b74
part 2 - Remove IsExtensible check from NativeObject::putProperty. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/688e161aa9f1
part 3 - Optimize UpdateShapeTypeAndValue. r=till
Assignee

Comment 9

2 years ago
These patches are just the tip of the iceberg, more patches soon.
Keywords: leave-open
Assignee

Updated

2 years ago
Blocks: emberperf
Assignee

Comment 11

2 years ago
Makes sure CallAddPropertyHook and CallAddPropertyHookDense get inlined. This also changes CallAddPropertyHook to take a HandleId instead of HandleShape, so we can remove a Shape root in AddOrChangeProperty.
Attachment #8853973 - Flags: review?(till)
Assignee

Comment 12

2 years ago
This templatizes AddOrChangeProperty so we can call addProperty instead of the slower putProperty if we know we're adding a new property.
Attachment #8853979 - Flags: review?(till)
Assignee

Comment 13

2 years ago
This inlines NativeObject::getChildPropertyOnDictionary into NativeObject::getChildProperty. This is faster because it eliminates a call that wasn't inlined, but it also makes the code easier to understand.
Attachment #8853987 - Flags: review?(till)
Comment on attachment 8853973 [details] [diff] [review]
Part 4 - Optimize CallAddPropertyHook*

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

r=me
Attachment #8853973 - Flags: review?(till) → review+
Comment on attachment 8853979 [details] [diff] [review]
Part 5 - Optimize AddOrChangeProperty

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

Nice, r=me
Attachment #8853979 - Flags: review?(till) → review+
Comment on attachment 8853987 [details] [diff] [review]
Part 6 - Inline NativeObject::getChildPropertyOnDictionary

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

r=me with or without the below question addressed.

::: js/src/vm/Shape.cpp
@@ +344,5 @@
> +    Shape* shape = cx->zone()->propertyTree().getChild(cx, parent, child);
> +    if (!shape)
> +        return nullptr;
> +    //MOZ_ASSERT(shape->parent == parent);
> +    //MOZ_ASSERT_IF(parent != lastProperty(), parent == lastProperty()->parent);

Preexisting, but should these be either removed or activated?
Attachment #8853987 - Flags: review?(till) → review+
Assignee

Comment 17

2 years ago
We want to make sure setLastProperty and updateSlotsForSpan get inlined in NativeObject::getChildProperty in Shape.cpp

The common case is that we don't need to realloc slots and the call overhead dominates these calls currently.
Attachment #8853998 - Flags: review?(till)
Comment on attachment 8853998 [details] [diff] [review]
Part 7 - Inline updateSlotsForSpan and setLastProperty

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

r=me, assuming the functions were moved without change. (Which seems to be the case, but I didn't verify completely.)
Attachment #8853998 - Flags: review?(till) → review+
Assignee

Comment 19

2 years ago
Posted patch Part 8 - Optimize IdIsIndex (obsolete) — Splinter Review
IdIsIndex is pretty hot, but it has an uninlined call to StringIsArrayIndex. This patch makes IdIsIndex check the first character of the string and if that's not 0-9 we can avoid the StringIsArrayIndex machinery.
Attachment #8854022 - Flags: review?(till)
Assignee

Comment 20

2 years ago
Oops this is the right one.
Attachment #8854022 - Attachment is obsolete: true
Attachment #8854022 - Flags: review?(till)
Attachment #8854023 - Flags: review?(till)
Comment on attachment 8854023 [details] [diff] [review]
Part 8 - Optimize IdIsIndex

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

r=me
Attachment #8854023 - Flags: review?(till) → review+
Assignee

Comment 22

2 years ago
They were commented out a few years ago but shell tests pass so let's just enable them again.
Attachment #8854447 - Flags: review?(till)
Comment on attachment 8854447 [details] [diff] [review]
Part 9 - Enable some commented out assertions

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

Sounds good.
Attachment #8854447 - Flags: review?(till) → review+

Comment 24

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3fbb45e9d5
part 4 - Optimize CallAddPropertyHook*. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/093562fdf1a1
part 5 - Optimize AddOrChangeProperty to call addProperty instead of putProperty when possible. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f789e78724
part 6 - Inline NativeObject::getChildPropertyOnDictionary into its only caller. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/be294387279d
part 7 - Ensure NativeObject::updateSlotsForSpan and NativeObject::setLastProperty get inlined. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/2512063d1c5d
part 8 - Optimize IdIsIndex to check the first character before calling StringIsArrayIndex. r=till
Assignee

Comment 25

2 years ago
Thanks for the fast reviews! There's still more to do here, will get back to it in a few days.
Nice, this was a measurable win on some six-speed tests.
Assignee

Comment 27

2 years ago
(In reply to Tom Schuster [:evilpie] from comment #26)
> Nice, this was a measurable win on some six-speed tests.

I think this push also improved Speedometer with 1 point or so on Windows, both 32-bit and 64-bit. There's a small drop after this landed. Not much but given how much code Speedometer runs that's actually more than I expected from these changes.
Assignee

Comment 29

2 years ago
I think I'm going to close this one. There's more we can do in bug 1368626.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED

Comment 30

2 years ago
Looks like Part 9 never landed. Can you please verify?
Flags: needinfo?(jdemooij)

Comment 31

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/312f0675cbc1
part 9 - Enable some assertions that were commented out a long time ago. r=till
Assignee

Comment 32

2 years ago
(In reply to Florian Bender from comment #30)
> Looks like Part 9 never landed. Can you please verify?

Hm yeah, not sure why I didn't push it. Thanks for the reminder.
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.