Fix MaybeInIteration/CanOptimizeForDenseStorage performance cliff

RESOLVED FIXED in Firefox 54

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Posted patch PatchSplinter Review
In MaybeInIteration we check if the object's group has the OBJECT_FLAG_ITERATED flag, but this means when an object is iterated once (or a different object with the same group), we will never use our optimized dense element paths for objects with that group.

This is causing some serious performance problems in bug 1342797 because array splice takes the slow path all the time.

I'm attaching a patch to optimize this code for when the compartment's enumerator list is empty or contains a single iterator that's for a different object.

It improves the 'shift' micro-benchmark below from > 3 seconds to 80 ms.

---

function f() {
    var a = [];
    for (var i in a) {}

    for (var i=0; i<100000; i++)
        a.push(i);

    var t = new Date;
    for (var i=0; i<1000; i++)
        a.shift();
    print(new Date - t);
        
}
f();
Attachment #8843268 - Flags: review?(andrebargull)
(Assignee)

Comment 1

2 years ago
I also removed the obj->isDelegate() check from MaybeInIteration and added a comment. Please double check that part.
(In reply to Jan de Mooij [:jandem] from comment #1)
> I also removed the obj->isDelegate() check from MaybeInIteration and added a
> comment. Please double check that part.

Looks okay to me. (I came to the same conclusion in bug 1323782, comment #3, but didn't bother to remove the check back then.)
Comment on attachment 8843268 [details] [diff] [review]
Patch

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

Cool! It also seems to speed up the delete operator.

If MaybeInIteration gets changed back to boolean returning function, CanOptimizeForDenseStorage can also be turned back into a boolean returning function.

::: js/src/jsarray.cpp
@@ +506,5 @@
> +        return false;
> +
> +    ObjectGroup* group = JSObject::getGroup(cx, obj);
> +    if (MOZ_UNLIKELY(!group)) {
> +        cx->recoverFromOutOfMemory();

In bug 1323782, I replaced the recoverFromOutOfMemory() call with JS::Result<>, because I assumed recoverFromOutOfMemory() is an escape hatch which is only used when it's not possible to properly propagate OOM exceptions. Now that you're reintroducing the recoverFromOutOfMemory() call, I'm wondering when it's okay to call recoverFromOutOfMemory() and when OOM exceptions should be propagated through the normal failure paths.
Attachment #8843268 - Flags: review?(andrebargull) → review+
(Assignee)

Comment 4

2 years ago
(In reply to André Bargull from comment #3)
> Now that you're reintroducing the recoverFromOutOfMemory() call,
> I'm wondering when it's okay to call recoverFromOutOfMemory() and when OOM
> exceptions should be propagated through the normal failure paths.

You may get different answers depending on who you ask, but IMO MaybeInIteration is the kind of simple function where it's fine to "return true" on OOM and take a less optimized path. Making this function fallible complicates things for the callers so IMO the recoverFromOutOfMemory call is worth it here.
(In reply to Jan de Mooij [:jandem] from comment #4)
> (In reply to André Bargull from comment #3)
> > Now that you're reintroducing the recoverFromOutOfMemory() call,
> > I'm wondering when it's okay to call recoverFromOutOfMemory() and when OOM
> > exceptions should be propagated through the normal failure paths.
> 
> You may get different answers depending on who you ask, but IMO
> MaybeInIteration is the kind of simple function where it's fine to "return
> true" on OOM and take a less optimized path. Making this function fallible
> complicates things for the callers so IMO the recoverFromOutOfMemory call is
> worth it here.

Okay, thanks! :-)

Comment 6

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2da8e0459adb
Fix performance cliff involving OBJECT_FLAG_ITERATED and array natives. r=anba

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2da8e0459adb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.