Closed Bug 918584 Opened 6 years ago Closed 6 years ago

PJS: Support SetProperty and SetElement ICs

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(4 files, 5 obsolete files)

Should be supported on thread local objects.
Assignee: general → shu
Attachment #807550 - Flags: review?(jdemooij)
Part 2 waiting on bug 903193, in case API changes.
Depends on: 903193
Attachment #807550 - Flags: review?(jdemooij) → review+
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)
Attachment #808075 - Flags: review?(jdemooij)
Attached patch Part 3: Add SetPropertyParIC (obsolete) — Splinter Review
Not flagging for r? yet, just putting it up.
Attached patch Part 4: Add SetElementParIC (obsolete) — Splinter Review
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+
Attachment #808075 - Flags: review?(jdemooij) → review+
Attached patch Part 3: Add SetPropertyParIC (obsolete) — Splinter Review
Attachment #808076 - Attachment is obsolete: true
Attachment #812316 - Flags: review?(jdemooij)
Attached patch Part 4: Add SetElementParIC (obsolete) — Splinter Review
Attachment #808077 - Attachment is obsolete: true
Attachment #812318 - Flags: review?(jdemooij)
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 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)
(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.
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)
Attachment #812839 - Flags: review?(jdemooij) → review+
Attachment #812369 - Flags: review?(jdemooij) → review+
Depends on: 925790
Duplicate of this bug: 876539
You need to log in before you can comment on or make changes to this bug.