Closed Bug 475474 Opened 15 years ago Closed 15 years ago

TM: maintain globalSlots per global, not just one per JIT instance


(Core :: JavaScript Engine, defect, P2)






(Reporter: gal, Assigned: graydon)



(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)


(2 files, 1 obsolete file)

Currently we maintain the current shape of the global object and a list of slots shared by all trees. If the global shape changes, we throw away the slot list and flush the JIT cache.

We want to track multiple globals, each with its own slot list. The recorder determines what slot list to use. Trees get a side pointer that points to their corresponding slot list for fast tree activation. All trees associated with the same global and the same shape of it share a slot list.

When jumping between trees, we have to be careful to only call trees with an identical slot list. The outermost tree decides what slot list will be used to unwind the native stack.

Also, keep in mind that slot lists grow as we discover more global variables being used. Trees extend their typemap accordingly, further specializing themselves towards the newly observed slots and their types.
Blocks a blocker, so please b+.
Flags: blocking1.9.1?
Priority: -- → P2
Flags: blocking1.9.1? → blocking1.9.1+
Attached patch First sketch (obsolete) — Splinter Review
Here's a preliminary sketch of what I *think* you mean; there's a lot wrong with it. It doesn't quite compile yet -- couple places I haven't figured out how to thread the right information in -- and there are a few places I'm certain I'm doing something wrong, marked. 

Just wanted to toss this up here for some initial feedback while I'm mopping it up, as I'm out of time for today. Andreas: is this about what you had in mind? Any thoughts on the approach?
Attached patch Update the patchSplinter Review
Here's an update to the patch that actually works. Ish. It passed sunspider and trace-test earlier this evening, but has regressed due to some changes on-branch that I haven't worked out; now it only passes sunspider.

It appears to grant a modest speedup: sunspider goes 1540 -> 1530 on the mac. Also seems to be slightly sensitive to turning the knob that says how many global shapes we can handle before flushing. If I track down the residual bug, I'll wiggle it around to see what a good setting is for trace-test, at least.

I'd appreciate very careful review. This is invasive.
Attachment #359207 - Attachment is obsolete: true
Attachment #359442 - Flags: review?(danderson)
Actually there was just breakage on the branch ( landing of bug 469625 ); if you apply to 9d9412b90552, just before that landing, you should be able to pass trace-test as well (minus 1 change to a trace stat, that seems to me to reflect "fewer flushes", so possibly harmless).
Comment on attachment 359442 [details] [diff] [review]
Update the patch

>             if (from != f) {
>+                // FIXME: there is no way I got this right.
>                 ti->dependentTrees.addUnique(from);
>-                ti->typeMap.captureMissingGlobalTypes(cx, *tm->globalSlots, ti->stackSlots);
>-            }
>-            ti = (TreeInfo*)from->vmprivate;
>-            ti->typeMap.captureMissingGlobalTypes(cx, *tm->globalSlots, ti->stackSlots);
>+                ti->typeMap.captureMissingGlobalTypes(cx, *from_ti->globalSlots, ti->nStackTypes);


>+            }
>+            // FIXME: or this.
>+            from_ti->typeMap.captureMissingGlobalTypes(cx, *ti->globalSlots, ti->nStackTypes);


It's trying to update both slot lists, so I think that should work.  r=me with that
Attachment #359442 - Flags: review?(danderson) → review+
Oops, a couple lines here should have made it into the initial patch. Quick r?
Attachment #359660 - Flags: review?(gal)
Attachment #359660 - Flags: review?(gal) → review?(danderson)
Attachment #359660 - Flags: review?(danderson) → review+
Followup landed as

Should be done now. Note to anyone porting this to another repo: you need to take both revs mentioned here, not just the last one.
Closed: 15 years ago
Resolution: --- → FIXED
Shouldn't this be marked fixed-in-tracemonkey until it lands on m-c?
Yes =)
Resolution: FIXED → ---
Whiteboard: fixed-in-tracemonkey
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.