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

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: gal, Assigned: graydon)

Tracking

({fixed1.9.1})

unspecified
x86
Mac OS X
fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Blocks a blocker, so please b+.
Flags: blocking1.9.1?
Priority: -- → P2

Updated

10 years ago
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Comment 2

10 years ago
Created attachment 359207 [details] [diff] [review]
First sketch

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?
(Assignee)

Comment 3

10 years ago
Created attachment 359442 [details] [diff] [review]
Update the patch

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)
(Assignee)

Comment 4

10 years ago
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);

s/from_ti/ti

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

s/ti/from_ti

It's trying to update both slot lists, so I think that should work.  r=me with that
Attachment #359442 - Flags: review?(danderson) → review+
(Assignee)

Comment 7

10 years ago
Created attachment 359660 [details] [diff] [review]
Minor followup, trivial warning and test-stat fixes.

Oops, a couple lines here should have made it into the initial patch. Quick r?
Attachment #359660 - Flags: review?(gal)
(Assignee)

Updated

10 years ago
Attachment #359660 - Flags: review?(gal) → review?(danderson)
Attachment #359660 - Flags: review?(danderson) → review+
(Assignee)

Comment 8

10 years ago
Followup landed as http://hg.mozilla.org/tracemonkey/rev/c3c596b0c5a3

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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Shouldn't this be marked fixed-in-tracemonkey until it lands on m-c?
(Reporter)

Comment 10

10 years ago
Yes =)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-tracemonkey

Comment 11

10 years ago
http://hg.mozilla.org/mozilla-central/rev/81a29bf64b16
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Reporter)

Updated

10 years ago
Duplicate of this bug: 473689

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.