Optimize the @@toStringTag and @@toPrimitive lookups

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Posted patch Patch (obsolete) — Splinter Review
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();
--
Attachment #8872996 - Flags: review?(evilpies)
(Assignee)

Comment 1

2 years ago
Posted patch PatchSplinter Review
Forgot to qref.
Attachment #8872996 - Attachment is obsolete: true
Attachment #8872996 - Flags: review?(evilpies)
Attachment #8873067 - Flags: review?(evilpies)
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?
Attachment #8873067 - Flags: review?(evilpies) → review+
Whiteboard: [qf] → [qf:p1]

Comment 3

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab80eaba78c
Optimize @@toStringTag and @@toPrimitive property lookups in the VM. r=evilpie
(Assignee)

Comment 4

2 years ago
(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

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