1369042 - Optimize the @@toStringTag and @@toPrimitive lookups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Object.prototype.toString and OrdinaryToPrimitive are pretty hot functions, but they have to do slow symbol lookups, even though in practice these symbols are almost never defined on the object.

The attached patch optimizes this as follows:

* When we add an "interesting symbol" property (currently @@toStringTag or @@toPrimitive) to an object, we set a flag on the BaseShape.

* When we look up these symbol properties, we can check this flag and avoid the slow lookup if it's not set.

The toString micro-benchmark below improves from 250 ms to 120 ms with this:
--
function f() {
    var res = "";
    var a = [1, 2, 3];
    var toString = Object.prototype.toString;
    var t = new Date;
    for (var i = 0; i < 5000000; i++)
	res = toString.call(a);
    print(new Date - t);
    return res;
}
f();
--

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Forgot to qref.

Comment 2

[Tom S [:evilpie]]

Comment on attachment 8873067 [details] [diff] [review]
Patch

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

Great results for such a relatively simple change!

::: js/src/jsobjinlines.h
@@ +290,5 @@
> +js::GetInterestingSymbolProperty(JSContext* cx, HandleObject obj, Symbol* sym, MutableHandleValue vp)
> +{
> +    JSObject* holder;
> +    if (!MaybeHasInterestingSymbolProperty(cx, obj, sym, &holder)) {
> +        vp.setUndefined();

I would assert that GetProperty really returns undefined here.

@@ +469,5 @@
>      return hasAllFlags(js::BaseShape::INDEXED);
>  }
>  
> +MOZ_ALWAYS_INLINE bool
> +JSObject::hasInterestingSymbolProperty() const

Add maybe maybe? (For the non-native/unboxed cases)

::: js/src/vm/Shape.cpp
@@ +683,5 @@
>  
>      Rooted<UnownedBaseShape*> nbase(cx);
>      {
> +        RootedShape shape(cx, obj->lastProperty());
> +        nbase = GetBaseShapeForNewShape(cx, shape, id);

Seems like before we would never use the last unowned baseshape here?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab80eaba78c
Optimize @@toStringTag and @@toPrimitive property lookups in the VM. r=evilpie

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #2)
> I would assert that GetProperty really returns undefined here.

Done.

> Add maybe maybe? (For the non-native/unboxed cases)

Done.

> Seems like before we would never use the last unowned baseshape here?

Yeah we would do a lookup and usually get the same BaseShape, so this is a small optimization. This code path (changing attributes of an existing property) is not very hot AFAIK but it can't hurt.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ab80eaba78c

Comment 6

[Vladan Djeric (:vladan)]

Looks like V8 borrowed this idea :)
https://v8project.blogspot.com/2017/09/v8-release-62.html