Allow nanojit cache size to be specified at runtime and on avmshell commandline

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: stejohns, Assigned: stejohns)

Tracking

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
Allows us to experiment with different cache sizes without rebuilding.
(Assignee)

Comment 1

11 years ago
Created attachment 328761 [details] [diff] [review]
Patch

Added the ability to specify the nanojit cache limit at runtime, either via the Configuration struct, or by a commandline argument in avmshell: -jitcachemax N, where N is a power of two (24 == 2^24 == 16MB).

Also added capability in Fragmento to track the high-water mark of JIT page usage, and added a flag to avmshell to dump this to stdout on exit (-jitcachelog). It's also dumped as part of -Dstats.

This turned up an interesting issue: GCC doesn't enforce that the Interpreter stack starts on an 8-byte boundary (which causes severe performance degradation). This smells like a compiler bug to me (or at least a questionable behavior), so I changed stack and rstack to be dynamically allocated (since MMgc guarantees 8-aligned blocks).

Also fixed a minor bug in performance/runtests.py.
Assignee: nobody → stejohns
Status: NEW → ASSIGNED
Attachment #328761 - Flags: review?(edwsmith)
(Assignee)

Updated

11 years ago
Attachment #328761 - Flags: review?(gal)

Comment 2

11 years ago
Comment on attachment 328761 [details] [diff] [review]
Patch

I know it's cosmetic but can we rename NJ_PAGES and NJ_UNLIMITED_GROWTH since they aren't #defines anymore.
Attachment #328761 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 3

11 years ago
pushed as changeset:   490:a58e8acf65a2
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 4

11 years ago
My change of yesterday caused a potential change in behavior if you specified "unlimited growth" for the jit cache (which is the default in avm.cpp):

We used to start with a big chunk of memory (16MB on Windows), and allow adding more big chunks, forever. But now “unlimited” starts with a tiny chunk (1 page, 4k) and adds tiny chunks forever... 

This is all fine and dandy until the tiny chunk isn’t reachable by a 24-bit delta, whereupon it gives up with an out-of-memory error and blacklists that trace.

oops.

Anyway, there's various ways to slice this fix, but since https://bugzilla.mozilla.org/show_bug.cgi?id=443111 is also attempting to address things related to this, I propose the short-term fix of removing "unlimited growth" as an option and going back to the explicit cache-size limits. 

(IMHO the unlimited-growth option was really only useful as an experimental tool and has outlived its usefulness; if someone really wants to try huge cache sizes, let them request 1GB or so as a cache size.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 5

11 years ago
Also, upon further reflection, it seems that the -jitcachelog option is really only interesting for relatively trivial apps.

currently, the way the JIT cache works is that we keep creating new traces until we run out of cache memory, then flush and restart... we never attempt to selectively flush traces.

therefore, if you specify a hard limit, we’ll always use all of it eventually -- unless the test is small enough that the total number of traces will never exceeed.

(Assignee)

Comment 6

11 years ago
Created attachment 328995 [details] [diff] [review]
Remove unlimited-growth option

Proposed patch to remove unlimited-growth option.
Attachment #328995 - Flags: review?(edwsmith)

Comment 7

11 years ago
Comment on attachment 328995 [details] [diff] [review]
Remove unlimited-growth option

looks good

what we really want is "limited growth", start small, grow without flushing until the limit is reached, then flush as needed.
Attachment #328995 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 8

11 years ago
Yep. That's gonna be hard to do reliably with the current 24-bit offset limitation -- if 8-byte LIR pans out, can we just embed a pointer instead?
(Assignee)

Comment 9

11 years ago
pushed as changeset:   492:88ec5fd8e0de -- let's see if this improves the buildbot.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #328761 - Flags: review?(gal)
You need to log in before you can comment on or make changes to this bug.