1058761 - Watch ArrayBufferObject for state changes rather than views in IonBuilder

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Right now when an ArrayBufferObject's data pointer changes due to neutering or other operations, this change triggers code invalidation via constraints attached to the views on that buffer.  As part of bug 1058340 we need to remove the views list attached to the buffer, so will no longer be able to obtain pointers to the views and trigger invalidations based on them.  The attached patch rejiggers this stuff so that the constraints are attached to the array buffer itself instead, and invalidations can be triggered without needing to walk the list of views.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 8479194 [details] [diff] [review]
patch

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

I should look at this.  I have some typed array neutering tests I haven't made time to land yet, and this should run the gauntlet.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8479194 [details] [diff] [review]
patch

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

Looks good but I agree Waldo should probably also look at this.

::: js/src/jit/IonBuilder.cpp
@@ +7544,5 @@
> +
> +        // The access is on a known object, try to embed the elements/length.
> +        TypedArrayObject *tarr = &tarrObj->as<TypedArrayObject>();
> +
> +        ArrayBufferObject *buf = tarr->buffer();

Do we have to check for |buf| being in the nursery? Maybe the function I mentioned in the other comment could assert it's not.

@@ +7868,5 @@
>  
>      TypedArrayObject *tarr = &tarrObj->as<TypedArrayObject>();
>  
> +    ArrayBufferObject *buf = tarr->buffer();
> +    if (!buf || !buf->hasSingletonType() || buf->isNeutered())

These checks appear at least 3 times, we could add a "static bool" helper function and call that instead.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Do we have to check for |buf| being in the nursery? Maybe the function I
> mentioned in the other comment could assert it's not.

ArrayBufferObjects and their data are no longer nursery allocatable, as of bug 979480.  I'll add a helper function and an assert.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: updated

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8480804 [details] [diff] [review]
updated

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

::: js/src/jit/IonBuilder.cpp
@@ +7156,2 @@
>      // LoadTypedArrayElementStatic currently treats uint32 arrays as int32.
> +    if (arrayType == Scalar::Uint32)

elementType seems a better name, if you're open to changing it.

@@ +7162,5 @@
>          return true;
>  
> +    TypedArrayObject *tarr = canEmbedTypedArray(obj);
> +    if (!tarr)
> +        return true;

Seems mildly nicer to move this before the convertShiftMumble call above, to my mind.  No reason to convert shifts or anything at all, if the data pointer isn't embeddable.

::: js/src/vm/ArrayBufferObject.cpp
@@ +324,4 @@
>          view->neuter(newContents.data());
>  
> +    // Notify compiled jit code that the base pointer has moved.
> +    MarkObjectStateChange(cx, buffer);

This is the wrong placement for this, because at this point you haven't actually changed the buffer's data pointer or byte length.  This needs to move down below the byteLength call and the potential setNewOwnedData call.  And really, there's not much reason it can't go below the setViewList/setIsNeutered calls as well, so below that entire block of adjustments.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: patch

Oops, good catch.

Comment 8

[Brian Hackett [Laid off!]]

Also, canEmbedTypedArray can add constraints triggering invalidation, while the other calls don't.  So the later it happens the better.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Brian Hackett (:bhackett) from comment #8)
> Also, canEmbedTypedArray can add constraints triggering invalidation, while
> the other calls don't.  So the later it happens the better.

Hmm, right.  I wonder if there isn't some better way to organize this, such that the constraint is added late, *after* the index stuff happens.  Eh, dilatory enough to not worry about here.

Comment 10

[Lars T Hansen [:lth]]

In these and other patches please be aware that I'm working on disentangling ArrayBuffer and SharedArrayBuffer (see bug 1054841 and dependent bugs and linked specs) and anything that ties those two closely together is likely to be thrown out shortly.

In general, it would be helpful if we could coordinate work on ArrayBuffer, I have a large patch queue that I need to land soon.

Comment 11

[Brian Hackett [Laid off!]]

With the new approach for bug 1058340 outlined in bug 1061404, this change isn't necessary anymore.