Closed Bug 852092 Opened 12 years ago Closed 12 years ago

Improve DOM list ICs

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch v1 (obsolete) (deleted) — Splinter Review
We should be more clever about when we want to generate a stub. Right now, at the time that we generate the stub, if the object has an expando that shadows then we generate a stub that bails out if there's an expando. Which means that if we loop over a list that has an expando we keep bailing out and regenerating a stub until we hit the max number of stubs. I think we should just not generate a stub if we have an object with a shadowing expando. The Ion version of the IC also doesn't have support for slot reads, which means it fails for all DOM methods.
Attached patch Part 1 v1 (deleted) — Splinter Review
This just factors out the code that generates the guards for the DOM lists in the Ion IC. This only moves and reindents code (and changes one comment).
Attachment #726100 - Attachment is obsolete: true
Attachment #729143 - Flags: review?(jdemooij)
Attached patch Part 2 v1 (deleted) — Splinter Review
This does two things, it stops us from generating stubs for objects that we know have an expando (so that we don't generate a stub and then immediately hit the guard and generate another stub and so on) and it make the Ion IC work for slot reads on the prototypes of ListBase proxies (all DOM methods are stored in slots on the prototypes).
Attachment #729155 - Flags: review?(jdemooij)
Comment on attachment 729143 [details] [diff] [review] Part 1 v1 Review of attachment 729143 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +334,5 @@ > CodeOffsetJump exitOffset; > CodeOffsetJump rejoinOffset; > CodeOffsetLabel stubCodePatchOffset; > > + void generateListBaseChecks(JSContext *cx, MacroAssembler &masm, JSObject *obj, Nit: move this out of the class: static void GenerateListBaseChecks(...) { } (This patch will conflict with bug 849469, which refactored the other methods.) @@ +335,5 @@ > CodeOffsetJump rejoinOffset; > CodeOffsetLabel stubCodePatchOffset; > > + void generateListBaseChecks(JSContext *cx, MacroAssembler &masm, JSObject *obj, > + PropertyName *propName, Register object, Label &stubFailure) Nit: use "Label *" for consistency with the other functions.
Attachment #729143 - Flags: review?(jdemooij) → review+
Comment on attachment 729155 [details] [diff] [review] Part 2 v1 Review of attachment 729155 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: js/src/ion/IonCaches.cpp @@ +404,5 @@ > failures); > > + bool isCacheableListBase = IsCacheableListBase(obj); > + Label listBaseFailures; > + if (isCacheableListBase) { Nit: single line body so no { }. @@ +526,1 @@ > } Nit: missed it in the previous patch, but no {} here either. @@ +898,4 @@ > { > // With Proxies, we cannot garantee any property access as the proxy can > // mask any property from the prototype chain. > + if (!checkObj->isProxy()) At the start of function we do: if (!checkObj || !checkObj->isNative()) return false; Proxies are non-native, so you can turn this into an assert: JS_ASSERT(!checkObj->isProxy());
Attachment #729155 - Flags: review?(jdemooij) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 869040
Depends on: 869796
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: