IonMonkey: Implement IC setter calls for JSNative setters

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla26
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Currently, we only generate stubs to call StrictPropertyOp setters, which isn't really all that useful for the DOM, whose objects are littered with JSNative setters with the new bindings.
(Assignee)

Comment 1

5 years ago
Created attachment 790498 [details] [diff] [review]
Part 1: Change OOLNativeGetterExitFrame to OOLNativeExitFrame
Attachment #790498 - Flags: review?(kvijayan)
(Assignee)

Comment 2

5 years ago
Created attachment 790499 [details] [diff] [review]
Part 2: Implement cacheing of JSNative setters in SetPropertyIC
Attachment #790499 - Flags: review?(kvijayan)
Comment on attachment 790498 [details] [diff] [review]
Part 1: Change OOLNativeGetterExitFrame to OOLNativeExitFrame

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

::: js/src/jit/IonFrames.cpp
@@ +937,5 @@
> +    if (frame.isOOLNative()) {
> +        IonOOLNativeExitFrameLayout *oolnative = frame.exitFrame()->oolNativeExit();
> +        gc::MarkIonCodeRoot(trc, oolnative->stubCode(), "ion-ool-native-code");
> +        size_t len = oolnative->argc() + 2;
> +        gc::MarkValueRootRange(trc, len, oolnative->vp(), "ion-ool-native-args");

For clarity's sake, I think this would be better split into a single MarkValueRoot(oolnative->vp()), plus an additional MarkValueRootRange(oolnative->argc() + 1, oolnative->thisp());

Alternatively, the IonFrames-ARCH.h files can be modified to explicitly insist that thisv and argvp immediately follow the callee result value stored in the frame.
Attachment #790498 - Flags: review?(kvijayan) → review+
Comment on attachment 790499 [details] [diff] [review]
Part 2: Implement cacheing of JSNative setters in SetPropertyIC

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

::: js/src/jit/IonCaches.cpp
@@ +1935,5 @@
> +    if (!shape || !IsCacheableProtoChain(obj, holder))
> +        return false;
> +
> +    if (!shape->writable())
> +        return false;

writable() is an attribute that is only relevant for data properties, not accessor (i.e. getter/setter) properties.

Drop this check.

@@ +1958,5 @@
> +    if (shape->hasDefaultSetter())
> +        return false;
> +
> +    if (!shape->writable())
> +        return false;

Remove writable() check.  See above.
Attachment #790499 - Flags: review?(kvijayan) → review+
https://hg.mozilla.org/mozilla-central/rev/587ec77a2373
https://hg.mozilla.org/mozilla-central/rev/6f82c0e76f2d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.