Closed
Bug 918584
Opened 11 years ago
Closed 11 years ago
PJS: Support SetProperty and SetElement ICs
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: shu, Assigned: shu)
References
Details
Attachments
(4 files, 5 obsolete files)
23.93 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
7.43 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
15.51 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
37.38 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Should be supported on thread local objects.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee: general → shu
Attachment #807550 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•11 years ago
|
||
Part 2 waiting on bug 903193, in case API changes.
Depends on: 903193
Updated•11 years ago
|
Attachment #807550 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 3•11 years ago
|
||
Sorry for the review churn Jan, but I cleaned up a the method to add new slots a bit. Previously it was using 3 shapes: the old shape before the add, the new shape after the add, and the property shape. In all cases, since this is an add, the new shape after the add should always be == the property shape == obj->lastProperty(), so we don't need to pass all 3 shapes along.
Attachment #807550 -
Attachment is obsolete: true
Attachment #808074 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #808075 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•11 years ago
|
||
Not flagging for r? yet, just putting it up.
Assignee | ||
Comment 6•11 years ago
|
||
Comment 7•11 years ago
|
||
Comment on attachment 808074 [details] [diff] [review] Part 1: Refactor SetPropertyIC Review of attachment 808074 [details] [diff] [review]: ----------------------------------------------------------------- Looks great; sorry for the delay.
Attachment #808074 -
Flags: review?(jdemooij) → review+
Updated•11 years ago
|
Attachment #808075 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #808076 -
Attachment is obsolete: true
Attachment #812316 -
Flags: review?(jdemooij)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #808077 -
Attachment is obsolete: true
Attachment #812318 -
Flags: review?(jdemooij)
Assignee | ||
Comment 10•11 years ago
|
||
fix bug in update with not calling the VM function when !canAttachStub()
Attachment #812318 -
Attachment is obsolete: true
Attachment #812318 -
Flags: review?(jdemooij)
Attachment #812369 -
Flags: review?(jdemooij)
Comment 11•11 years ago
|
||
Comment on attachment 812316 [details] [diff] [review] Part 3: Add SetPropertyParIC Review of attachment 812316 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but SetPropertyCache is very common and an extra temp register is unfortunate. Do we only need this for PJS? In that case, can we use LDefinition::BogusTemp() in the sequential execution case and rename the temp() method on the LIR to something that makes it clear it's for PJS only?
Attachment #812316 -
Flags: review?(jdemooij)
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #11) > Comment on attachment 812316 [details] [diff] [review] > Part 3: Add SetPropertyParIC > > Review of attachment 812316 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, but SetPropertyCache is very common and an extra temp register > is unfortunate. Do we only need this for PJS? In that case, can we use > LDefinition::BogusTemp() in the sequential execution case and rename the > temp() method on the LIR to something that makes it clear it's for PJS only? Sure, will do.
Assignee | ||
Comment 13•11 years ago
|
||
Refactored the dispatch cache temp stuff a bit. Note that Part 4 is unchanged, as SetElementCache already had its share of temps that I could reuse.
Attachment #812316 -
Attachment is obsolete: true
Attachment #812839 -
Flags: review?(jdemooij)
Updated•11 years ago
|
Attachment #812839 -
Flags: review?(jdemooij) → review+
Updated•11 years ago
|
Attachment #812369 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 14•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0e8a18ec9d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce1959c9b3cb remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec6607f70f52 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7924e5fb323a
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0c0e8a18ec9d https://hg.mozilla.org/mozilla-central/rev/ce1959c9b3cb https://hg.mozilla.org/mozilla-central/rev/ec6607f70f52 https://hg.mozilla.org/mozilla-central/rev/7924e5fb323a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•