881536 - PJS: Support GetElementIC in parallel

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

Should be fairly straightforward.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Part 1: Factor out stubbedShape to ParallelIonCache

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Part 2: Refactor initializeAddCacheState into DispatchIonCache

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Part 3: Pure VM path for GetElement

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Part 1: Factor out stubbedShape to ParallelIonCache

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Part 3: Pure VM path for GetElement

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Part 4: Implement ParallelGetElementIC

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 760672 [details] [diff] [review]
Part 3: Pure VM path for GetElement

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

::: js/src/jsobj.cpp
@@ +4131,5 @@
> +    /* Fast-path typed array index accesses. */
> +    if (obj->isTypedArray() && index < TypedArray::length(obj)) {
> +        TypedArray::copyTypedArrayElement(obj, index, MutableHandleValue::fromMarkedLocation(vp));
> +        return true;
> +    }

This looks like more than just a fast path, it seems GetPropertyPure will fail if passed a typed array due to typed arrays being non-native classes.  Could this special casing be done in GetPropertyPure / LookupPropertyPure themselves?  It seems like looking for typed arrays there would be more robust and allow accesses like typedArray.length to work in PJS.

@@ +4148,5 @@
> +    uint32_t index;
> +    if (IsDefinitelyIndex(prop, &index))
> +        return GetElementPure(obj, index, vp);
> +
> +    /* Atomizing the property value is effectful and not threadsafe. */

(well, not yet anyways)

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 760671 [details] [diff] [review]
Part 1: Factor out stubbedShape to ParallelIonCache

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

This is a good patch which extract the ShapeSet and the logic for attaching stubs to a ParallelIonCache.  For parallel caches which will not rely on shapes this will just create an extra NULL pointer, but this sounds fine for now.

::: js/src/ion/IonCaches.h
@@ +881,5 @@
> +    bool initStubbedShapes(JSContext *cx);
> +
> +  public:
> +    virtual void reset();
> +    virtual void destroy();

nit: Usually we do not repeat the virtual keyword.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 760688 [details] [diff] [review]
Part 4: Implement ParallelGetElementIC

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

Good job, thanks for refactoring the can* functions and use them in assertions of the code-gen part of ICs.

::: js/src/ion/IonCaches.cpp
@@ +2854,5 @@
> +
> +    // Avoid unnecessary locking if cannot attach stubs and idempotent.
> +    if (cache.idempotent() && !cache.canAttachStub())
> +        return TP_SUCCESS;
> +    {

nit: This is weird. Either add a space before the curly brace or remove them.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 760688 [details] [diff] [review]
Part 4: Implement ParallelGetElementIC

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

I think you might need to add ParallelElementIC::initializeAddCacheState in CodeGenerator-x86.cpp

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> Comment on attachment 760688 [details] [diff] [review]
> Part 4: Implement ParallelGetElementIC
> 
> Review of attachment 760688 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you might need to add ParallelElementIC::initializeAddCacheState in
> CodeGenerator-x86.cpp

Ah, good catch!

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> nit: Usually we do not repeat the virtual keyword.

MOZ_OVERRIDE is a better idea, and it makes sure signature changes happen properly if the base class signature changes.

Comment 13

[Shu-yu Guo [:shu]]

Attachment: Part 3: Pure VM path for GetElement

Hopefully addressed comments.

Comment 14

[Shu-yu Guo [:shu]]

Part 3 here depends on part 1 of bug 881574.

Comment 15

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6222cdc410
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/50339ff1412f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b050f72961f0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cc695d611390

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb6222cdc410
https://hg.mozilla.org/mozilla-central/rev/50339ff1412f
https://hg.mozilla.org/mozilla-central/rev/b050f72961f0
https://hg.mozilla.org/mozilla-central/rev/cc695d611390