Closed
Bug 1384244
Opened 7 years ago
Closed 7 years ago
MaybeHasInterestingSymbolProperty has an unnecessary check for hasDynamicPrototype()
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(1 file)
2.44 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•7 years ago
|
||
Good find. Do you have time to fix it? :)
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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 4•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e537243fed2613487c724651f0b7e7473ed83eb
Keywords: checkin-needed
Assignee | ||
Comment 5•7 years 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•7 years 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?
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/74d5a19b2814
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 9•7 years ago
|
||
(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.
Description
•