1100757 - Avoid adding a shape guard on the proto in testCommonGetterSetter if the property is non-configurable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now testCommonGetterSetter adds a shape guard on foundProto.

But if the property is non-configurable, we know the getter/setter can't change.  So it seems like in that case we can omit the shape guard on the proto, no?

Can we tell in this code that the prop is non-configurable?  I mean, I guess we could stash that info in the baseline IC, but is there a better way?

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #0)
> Can we tell in this code that the prop is non-configurable?  I mean, I guess
> we could stash that info in the baseline IC, but is there a better way?

I'm not sure why the shape guard is necessary, but if you're right you could do something like this:

Shape *propShape = foundProto->lookupPure(name);
if (propShape && !propShape->configurable()) {
  ...
}

IonBuilder always runs on the main thread so it won't race. We don't have access to a JSContext though, that's why we use the lookupPure method. It might bail if it sees resolve or lookups hooks though...

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> It might bail if it sees resolve or lookups hooks though...

To be clear, "bail" here just means return nullptr, but I looked at the definition and lookupPure calls Shape::searchNoHashify; it doesn't check for class hooks.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #0)
> But if the property is non-configurable, we know the getter/setter can't
> change.  So it seems like in that case we can omit the shape guard on the
> proto, no?

In a sane world, sure.  In the broken SpiderMonkey world where the JSAPI does currently allow redefining non-configurable properties if you squint right, I am rather leery of doing this.  :-\

Comment 4

[Boris Zbarsky [:bzbarsky]]

> In the broken SpiderMonkey world where the JSAPI does currently allow redefining
> non-configurable properties if you squint right

Would you prefer it if I fixed that first?  The one place I know of where Gecko depends on it is going away in bug 1012798, but that causes a slight slowdown due to this bug, which I'd rather like to fix.

Comment 5

[Jeff Walden [:Waldo]]

Well, yes, I'd prefer it.  But it is seriously non-trivial, efaust has been more or less working on that sort of thing for months now with JSPROP_IGNORE* bits and other nonsense.  So I don't think you should be spending your time on it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  Filed bug 1101123 on fixing the JSAPI bit here.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't emit a guard in testCommonGetterSetter when our getter/setter is is a non-configurable property on a native object

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Comment 9

[Eric Faust [:efaust]]

Comment on attachment 8529317 [details] [diff] [review]
Merged to tip

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

With the JSPROP flag I just reviewed, this can now go in.

Comment 10

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4247f2646a63

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4247f2646a63