CacheIR: Set array length IC

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine: JIT
P3
normal
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
Created attachment 8831382 [details] [diff] [review]
CacheIR: Set array length IC

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 1

10 months ago
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+

Comment 2

10 months ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72188279b3a9
CacheIR: SetProp array length IC. r=jandem
Priority: -- → P3

Comment 3

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/72188279b3a9
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.