Closed Bug 983977 Opened 6 years ago Closed 6 years ago

Omit type barrier for derived typed objects where possible

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nmatsakis, Assigned: nmatsakis)

References

Details

Attachments

(1 file, 1 obsolete file)

Examination of bug 966567 reveals that inserting type barriers for derived typed objects causes us to fail to optimize away the derived typed objects, leading to bad performance and lack of parallelizability (the latter part is something that should be fixed separately -- it should be possible to allocate typed objects in parallel code, that's just not been done yet).

In any case, most of the time these type barriers are not needed. This patch examines the predicted types and determines whether the observed types are complete. This is possible because the TI type object for a typed object is a function of the class/prototype, and:

- the class of a derived typed object will always be the same as the typed object it is derived from (often we know this from TI);

- the prototype of a derived typed object is determined by the `prototype` field on its type descriptor, which is often known from the TypeDescrSet. Note that per the spec [1] the `prototype` field on type descriptors is read-only -- I added a specific test for this as well, since I don't think there was one before, and the way I wrote the code (at least) relies on this.

I also cooked up some micro-benchmarks I will independently submit to AWFY. This change leads to a massive increase in performance. (With the change, the benchmark takes 405 milliseconds to run. Without the change, I ran out of patience waiting for it to complete.)

[1] https://github.com/dslomov-chromium/typed-objects-es7
Blocks: 966567
Update: 29145 milliseconds without the change.
Attached patch Bug983977.diff (obsolete) — Splinter Review
Attachment #8391685 - Flags: review?(jdemooij)
Comment on attachment 8391685 [details] [diff] [review]
Bug983977.diff

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

Sorry for the delay. Nice perf win!

::: js/src/builtin/TypedObject.h
@@ +437,5 @@
>      // does `new StructType(...)`. It produces a struct type object.
>      static bool construct(JSContext *cx, unsigned argc, Value *vp);
>  };
>  
> +class StructTypeDescr : public ComplexTypeDescr {

Pre-existing nit: { on its own line.

::: js/src/jit/IonBuilder.cpp
@@ +6865,5 @@
> +    // opaque) will be the same as the incoming object from which the
> +    // derived typed object is, well, derived. The prototype will be
> +    // determined based on the type descriptor (and is immutable).
> +    types::TemporaryTypeSet *objTypes = obj->resultTypeSet();
> +    const Class *expectedClass = (objTypes ? objTypes->getKnownClass() : nullptr);

Nit: no parentheses.

::: js/src/tests/ecma_6/TypedObject/prototypes.js
@@ +1,1 @@
> +// |reftest| skip-if(!this.hasOwnProperty("TypedObject"))

Hm can we turn this into a jit-test, so that we also test it with --ion-eager etc?
Attachment #8391685 - Flags: review?(jdemooij) → review+
Attached patch Bug983977.diffSplinter Review
Rebased, carrying over r+ from jandem.

One non-nit change I made: if we omit the barrier, also set the ResultTypeSet on the MNewDerivedTypedObject. This helps avoid barriers for more complex paths.
Attachment #8391685 - Attachment is obsolete: true
Attachment #8393250 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/48037d26cc96
Assignee: nobody → nmatsakis
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.