Closed Bug 1100757 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(jdemooij)
(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...
Flags: needinfo?(jdemooij)
(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.
(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.  :-\
> 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.
Depends on: 1012798
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(jwalden+bmo)
Depends on: 1101123
OK.  Filed bug 1101123 on fixing the JSAPI bit here.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Depends on: 1101823
Attached patch Merged to tipSplinter Review
Attachment #8529317 - Flags: review?(efaustbmo)
Attachment #8525743 - Attachment is obsolete: true
Attachment #8525743 - Flags: review?(efaustbmo)
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.
Attachment #8529317 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/mozilla-central/rev/4247f2646a63
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1148973
You need to log in before you can comment on or make changes to this bug.