Closed Bug 1334727 Opened 3 years ago Closed 3 years ago

CacheIR: Set array length IC

Categories

(Core :: JavaScript Engine: JIT, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

SetProp with length on arrays is probably the most common missing stub I see, except for what I assume would be solved by the add property stub.

We talked about this before as well, and this probably the most common JSPropertyOp that is still relevant, so we won't need to support those.

This speeds up a stupid loop running in baseline that is just assigning to length on a new array from 20ms to 7ms.
Attachment #8831382 - Flags: review?(jdemooij)
Comment on attachment 8831382 [details] [diff] [review]
CacheIR: Set array length IC

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

Great! Thanks for doing this.

::: js/src/jit/CacheIR.h
@@ +756,4 @@
>      Scalar::Type scalarType() { return Scalar::Type(buffer_.readByte()); }
>      uint32_t typeDescrKey() { return buffer_.readByte(); }
>      JSWhyMagic whyMagic() { return JSWhyMagic(buffer_.readByte()); }
> +    bool boolean() { return bool(buffer_.readByte()); }

I added getBool for this in the AddSlot patch.

::: js/src/jit/VMFunctions.cpp
@@ +396,5 @@
>  
>  bool
> +SetArrayLength(JSContext* cx, HandleObject obj, HandleValue value, bool strict)
> +{
> +    Rooted<ArrayObject*> array(cx, &obj->as<ArrayObject>());

Nit: we can eliminate the Rooted with:

Handle<ArrayObject*> array = obj.as<ArrayObject>();

::: js/src/jit/VMFunctions.h
@@ +631,4 @@
>  MOZ_MUST_USE bool ArrayPushDense(JSContext* cx, HandleObject obj, HandleValue v, uint32_t* length);
>  MOZ_MUST_USE bool ArrayShiftDense(JSContext* cx, HandleObject obj, MutableHandleValue rval);
>  JSString* ArrayJoin(JSContext* cx, HandleObject array, HandleString sep);
> +bool SetArrayLength(JSContext* cx, HandleObject obj, HandleValue value, bool strict);

Nit: add MOZ_MUST_USE for consistency.
Attachment #8831382 - Flags: review?(jdemooij) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72188279b3a9
CacheIR: SetProp array length IC. r=jandem
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/72188279b3a9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.