MaybeHasInterestingSymbolProperty has an unnecessary check for hasDynamicPrototype()

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
I don't think we need to check |obj->hasDynamicPrototype()|, because |obj->maybeHasInterestingSymbolProperty()| always returns true for proxies.


[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/js/src/jsobjinlines.h#276
Good find. Do you have time to fix it? :)
(Assignee)

Comment 2

a year ago
Created attachment 8890893 [details] [diff] [review]
bug1384244.patch

I've also replaced the two append(char*) calls with append(char), because the latter is marginally faster.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8890893 - Flags: review?(jdemooij)
Comment on attachment 8890893 [details] [diff] [review]
bug1384244.patch

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

Nice, thanks.

::: js/src/jsobjinlines.h
@@ -277,1 @@
>              MOZ_UNLIKELY(ClassMayResolveId(cx->names(), obj->getClass(), id, obj) ||

(FWIW I've been thinking about setting the has-interesting-symbols BaseShape flag eagerly in the resolve hook case. That would let us get rid of this check... Not sure how much it matters...)
Attachment #8890893 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 5

a year ago
(In reply to Jan de Mooij [:jandem] from comment #3)
> ::: js/src/jsobjinlines.h
> @@ -277,1 @@
> >              MOZ_UNLIKELY(ClassMayResolveId(cx->names(), obj->getClass(), id, obj) ||
> 
> (FWIW I've been thinking about setting the has-interesting-symbols BaseShape
> flag eagerly in the resolve hook case. That would let us get rid of this
> check... Not sure how much it matters...)

Kind of related to the may-resolve check: Do ArgumentsObject show up often in obj_toString? Because the current may-resolve hook for ArgumentsObjects returns |true| for any symbol. http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/js/src/vm/ArgumentsObject.cpp#437-439
(Assignee)

Comment 6

a year ago
(In reply to Jan de Mooij [:jandem] from comment #3)
> ::: js/src/jsobjinlines.h
> @@ -277,1 @@
> >              MOZ_UNLIKELY(ClassMayResolveId(cx->names(), obj->getClass(), id, obj) ||
> 
> (FWIW I've been thinking about setting the has-interesting-symbols BaseShape
> flag eagerly in the resolve hook case. That would let us get rid of this
> check... Not sure how much it matters...)

This would only affect functions, arguments, and string objects (at least when only considering built-ins), correct?

Comment 7

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d5a19b2814
Remove hasDynamicPrototype check from MaybeHasInterestingSymbolProperty because only proxies can have dynamic prototypes. r=jandem
Keywords: checkin-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74d5a19b2814
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(In reply to André Bargull from comment #5)
> Kind of related to the may-resolve check: Do ArgumentsObject show up often
> in obj_toString? Because the current may-resolve hook for ArgumentsObjects
> returns |true| for any symbol.

I'm not sure how common they are. It's probably worth fixing...

(In reply to André Bargull from comment #6)
> > (FWIW I've been thinking about setting the has-interesting-symbols BaseShape
> > flag eagerly in the resolve hook case. That would let us get rid of this
> > check... Not sure how much it matters...)
> 
> This would only affect functions, arguments, and string objects (at least
> when only considering built-ins), correct?

It depends on how we implement this (we could whitelist these builtin classes). My idea was to set the BaseShape flag when we create the initial/empty shape if the Class has a resolve hook, so we don't have to worry about it each time we do a lookup. Instead of an explicit whitelist we could also just call ClassMayResolveId for @@toStringTag and @@toPrimitive at that point...
You need to log in before you can comment on or make changes to this bug.