Optimize calls to own property scripted getters/setters

RESOLVED FIXED in mozilla39

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bz, Assigned: jandem)

Tracking

unspecified
mozilla39
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Apparently, we only optimize scripted getters/setters if they're on the proto chain, not on the receiver.

We do handle own native getters/setters...

Comment 1

3 years ago
Ah, interesting. JSIL actually uses this a lot. I wonder if the same is true for V8...
V8 seems to optimize these fine.
(Assignee)

Comment 3

3 years ago
The patch in bug 1129382 adds Ion ICs for scripted getters/setters and these will also work for own getters/setters. The Baseline ICs and IonBuilder paths should also support own getters/setters, but it's a good start.
Depends on: 1129382
(Assignee)

Updated

3 years ago
Blocks: 626021
(Assignee)

Comment 4

3 years ago
Created attachment 8565409 [details] [diff] [review]
Part 1 - Rename GetProp_CallScripted to GetProp_CallScriptedPrototype

This just renames Baseline's GetProp_CallScripted stub to GetProp_CallScriptedPrototype, this nicely matches CallNativePrototype.

No change in behavior.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8565409 - Flags: review?(efaustbmo)
(Assignee)

Updated

3 years ago
Depends on: 1134142
(Assignee)

Updated

3 years ago
Depends on: 1134232

Comment 5

3 years ago
Comment on attachment 8565409 [details] [diff] [review]
Part 1 - Rename GetProp_CallScripted to GetProp_CallScriptedPrototype

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

Yeah, this probably should have happened when I did the native side refactor, for consistency. Good to see it happen now.
Attachment #8565409 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8572598 [details] [diff] [review]
Part 1 - Refactor + handle own scripted getters

Actually, I no longer think having separate stubs for own-getter and proto-getter is the way to go. It does save some memory but it adds a lot of extra code and complexity and I don't think it's worth it.

This patch merges GetProp_CallNative and GetProp_CallNativePrototype into GetProp_CallNative, and makes GetProp_CallScripted handle own getters. It removes at least 150 lines of code.

Also note that we currently only handle setters on the prototype, and we'd need the same inheritance structure there for scripted/native setters. With this approach, handling own setters should be a lot simpler.
Attachment #8565409 - Attachment is obsolete: true
Attachment #8572598 - Flags: review?(efaustbmo)

Comment 7

3 years ago
Comment on attachment 8572598 [details] [diff] [review]
Part 1 - Refactor + handle own scripted getters

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

This is a pretty reduction. r=me

Thanks for adding the various asserts about key firlds, and cleaning up Native code as well.
Attachment #8572598 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 8

3 years ago
Part 1, own scripted getters:

https://hg.mozilla.org/integration/mozilla-inbound/rev/83fabedf6d3a

Now I just need to do the same for setters.
Keywords: leave-open
(Assignee)

Updated

3 years ago
Depends on: 1157231
(Assignee)

Comment 10

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #8)
> Now I just need to do the same for setters.

Doing this in bug 1157231, to make release tracking easier (the patch here landed in 39).
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.