TM: lazily load stack slots and global values

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: gal, Assigned: dvander)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Updated

9 years ago
Assignee: general → gal
(Reporter)

Updated

9 years ago
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
(Reporter)

Comment 1

9 years ago
Created attachment 416623 [details] [diff] [review]
patch, WIP
(Reporter)

Comment 2

9 years ago
Created attachment 416651 [details] [diff] [review]
patch, WIP
Attachment #416623 - Attachment is obsolete: true
Created attachment 416658 [details] [diff] [review]
less broken, but still broken
Attachment #416651 - Attachment is obsolete: true
Created attachment 416740 [details] [diff] [review]
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.
Attachment #416658 - Attachment is obsolete: true
Created attachment 416842 [details] [diff] [review]
working

we can't lazily import boxed slots.

passes trace-tests now, no SS regressions.
Attachment #416740 - Attachment is obsolete: true
Attachment #416842 - Flags: review?(gal)
(Reporter)

Comment 6

9 years ago
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.
Attachment #416842 - Flags: review?(gal) → review+
Created attachment 417020 [details] [diff] [review]
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
Assignee: gal → dvander
Attachment #416842 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #417020 - Flags: review?(gal)
(Reporter)

Updated

9 years ago
Attachment #417020 - Flags: review?(gal) → review+
No longer blocks: 534745
Depends on: 534745

Comment 9

9 years ago
http://hg.mozilla.org/mozilla-central/rev/538a07fdf3d2
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 535930
Depends on: 546611
You need to log in before you can comment on or make changes to this bug.