Closed
Bug 1346217
Opened 7 years ago
Closed 7 years ago
Optimize adding/defining new properties on native objects
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Performance Impact | high |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 3 open bugs)
Details
Attachments
(9 files, 2 obsolete files)
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 |
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.
Updated•7 years ago
|
Whiteboard: [qf:investigate:p1]
Assignee | ||
Comment 1•7 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•7 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•7 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•7 years ago
|
||
Forgot to qref.
Attachment #8848093 -
Attachment is obsolete: true
Attachment #8848093 -
Flags: review?(till)
Attachment #8848094 -
Flags: review?(till)
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
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 7•7 years ago
|
||
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+
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•7 years ago
|
||
These patches are just the tip of the iceberg, more patches soon.
Keywords: leave-open
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/565beb14681c https://hg.mozilla.org/mozilla-central/rev/062ad3879b74 https://hg.mozilla.org/mozilla-central/rev/688e161aa9f1
Assignee | ||
Comment 11•7 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•7 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•7 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 14•7 years ago
|
||
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 15•7 years ago
|
||
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 16•7 years ago
|
||
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•7 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 18•7 years ago
|
||
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•7 years ago
|
||
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•7 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 21•7 years ago
|
||
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•7 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 23•7 years ago
|
||
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•7 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•7 years ago
|
||
Thanks for the fast reviews! There's still more to do here, will get back to it in a few days.
Comment 26•7 years ago
|
||
Nice, this was a measurable win on some six-speed tests.
Assignee | ||
Comment 27•7 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.
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e3fbb45e9d5 https://hg.mozilla.org/mozilla-central/rev/093562fdf1a1 https://hg.mozilla.org/mozilla-central/rev/f3f789e78724 https://hg.mozilla.org/mozilla-central/rev/be294387279d https://hg.mozilla.org/mozilla-central/rev/2512063d1c5d
Assignee | ||
Comment 29•7 years ago
|
||
I think I'm going to close this one. There's more we can do in bug 1368626.
Comment 30•7 years ago
|
||
Looks like Part 9 never landed. Can you please verify?
Flags: needinfo?(jdemooij)
Comment 31•7 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•7 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)
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/312f0675cbc1
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:investigate:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•