515749 - TM: lazily load stack slots and global values

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Andreas Gal :gal]

We currently emit a ton of loads at the top of the trace (loop), causing register pressure, forcing us to spill many of those loads straight back onto the stack. Instead, we should emit the loads on demand and closer to the first use.

Comment 1

[Andreas Gal :gal]

Attachment: patch, WIP

Comment 2

[Andreas Gal :gal]

Attachment: patch, WIP

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: less broken, but still broken

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: almost working

It seems like we can't clear the whole tracker after a tree call. Our stack has two areas with lazy "holes" in it: at the entry frame, and the frame at the tree call. When exiting the inner tree we only have have the innermost slots, but the import type map still has to express both. So either:

1) We clear the whole tracker, and re-lazily-import _everything_, or
2) We clear just the part of the tracker and leave dead space in the import type map.

If this thinking is right at all, there's basically no difference in complexity, so I went with #2. This patch fixes some other bugs as well.

We're still failing around 4 test cases though.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: working

we can't lazily import boxed slots.

passes trace-tests now, no SS regressions.

Comment 6

[Andreas Gal :gal]

Comment on attachment 416842 [details] [diff] [review]
working

> JS_REQUIRES_STACK void
> TraceRecorder::import(TreeFragment* tree, LIns* sp, unsigned stackSlots, unsigned ngslots,
>                       unsigned callDepth, JSTraceType* typeMap)
> {
>     /*
>+     * Either the tracker is already empty, or we are re-import and we want everything
>+     * to be reloaded (lazily). For the latter we clear the tracker to force the
>+     * re-import of every tracked value.
>+     */
>+    tracker.clear();
>+

The tracker should always be empty here.

>     /*
>+     * Flush values from the tracker which could have been invalidated by the
>+     * inner tree. This means variables local to this frame and global slots.
>+     */
>+    clearFrameSlotsFromTracker(tracker);
>+    SlotList& gslots = *tree->globalSlots;
>+    for (unsigned i = 0; i < gslots.length(); i++) {
>+        unsigned slot = gslots[i];
>+        jsval* vp = &STOBJ_GET_SLOT(globalObj, slot);
>+        tracker.set(vp, NULL);
>+    }

A comment here that the offset cache is still valid. We just flush the tracker to force reloads.

David wants to change side exits to pick the type from the initial map instead of always calling get() if a value isn't in the tracker. This should reduce unnecessary early imports in the first side exit.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: better patch

This version is about a 15ms perf win on my machine for SS. It avoids creating unnecessary stores which keep loads live.

http://pastebin.mozilla.org/689897

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/538a07fdf3d2

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/538a07fdf3d2