Optimize Object.prototype.toString more

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Bug 1369042 was a decent win, but there's a bunch more slowness to eliminate here.
(Assignee)

Comment 1

2 years ago
Posted patch PatchSplinter Review
This patch:

* Moves JSObject::callHook, JSObject::isCallable etc to jsobjinlines.h These functions are just a few branches in the common case so we don't want OOL calls.

* Adds a fast path to determine builtinTag in obj_toString. I moved the existing code into GetBuiltinTagSlow, that code is now only used for proxies. In debug builds we check these return the same result. This adds some duplication but considering how hot this code is I think it's worth it.

* I made the (most common) PlainObject case a bit faster by avoiding the strcmp.

For the micro-benchmark below I get:

before: 786 ms
after:  536 ms

So about a 30% improvement.

function f() {
    var res = "";
    var objs = [{}, [1, 2], new Date, function(){}];
    var toString = Object.prototype.toString;
    var t = new Date;
    for (var i = 0; i < 5000000; i++) {
        for (var j = 0; j < 4; j++)
            res = toString.call(objs[j]);
    }
    print(new Date - t);
    return res;
}
f();
Attachment #8881877 - Flags: review?(evilpies)
Comment on attachment 8881877 [details] [diff] [review]
Patch

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

I am a bit surprised that reimplementing GetBuiltinClass makes such a huge difference, is that just call overhead?

::: js/src/builtin/Object.cpp
@@ +519,3 @@
>      } else {
> +        // Optimize the non-proxy case to bypass GetBuiltinClass.
> +        if (clasp == &PlainObject::class_ || clasp == &UnboxedPlainObject::class_) {

This needs some comment about this being an optimization.

@@ -516,5 @@
>          if (!builtinTag) {
>              const char* className = GetObjectClassName(cx, obj);
> -            // "[object Object]" is by far the most common case at this point,
> -            // so we optimize it here.
> -            if (strcmp(className, "Object") == 0) {

Some other classes also use Object like Date.prototype, but it's probably not that common.
Attachment #8881877 - Flags: review?(evilpies) → review+
(Assignee)

Comment 3

2 years ago
(In reply to Tom Schuster [:evilpie] from comment #2)
> I am a bit surprised that reimplementing GetBuiltinClass makes such a huge
> difference, is that just call overhead?

Call overhead too but worst case GetBuiltinClass does 20 Class comparisons or so and then we return to obj_toString where we do a second switch on the ESClass value.

Comment 4

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c199e7e82e60
Optimize Object.prototype.toString. r=evilpie

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c199e7e82e60
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Significant effect on SMv2. Marking p1 for future reference.
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.