Closed Bug 489615 Opened 11 years ago Closed 11 years ago

TM: Store recording attempts in a long-lived hashtable rather than fragments.

Categories

(Core :: JavaScript Engine, defect, P2, major)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: graydon, Assigned: graydon)

Details

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

Attachments

(1 file, 4 obsolete files)

Currently we store the number of recording attempts for each PC in the root fragment of the tree at that PC. This has a number of problems, but the most glaring is that recording attempt counts don't survive a cache flush, so any PC that indirectly causes pathological flushing never gets blacklisted. It just keeps being re-recorded and re-flushing forever. This is an easy route to exhibiting slower-than-interp JIT performance.

Fix this by moving the recording attempt numbers into a separate hashtable.
Flags: blocking1.9.1?
Attached patch Sketch of solution (obsolete) — Splinter Review
This needs, at minimum, to also scan the hashtable when a script is unloaded and un-blacklist any PCs that were in range. Otherwise works.
(In reply to comment #1)
> This needs, at minimum, to also scan the hashtable when a script is unloaded
> and un-blacklist any PCs that were in range.

js_PurgeScriptFragments stands ready.

Drive-by from IRC: use JS_DHashGetStubOps() instead of rolling your own.

/be
I think this solves the problem correctly now. It doesn't seem to perturb any of the trace-tests. I'll run it through try servers presently to check it further, it's a little invasive.
Attachment #374097 - Attachment is obsolete: true
Attachment #374561 - Flags: review?(gal)
Woah, try servers *hate* that. Lots of explosions. Need to debug a bit here, I guess.
Attached patch Update the patch (obsolete) — Splinter Review
This variant appears to work much better. The burning was only caused by my silly placement of the hashtable init code in an assertion (thereby ensuring it never happened on opt builds). But I spent some time adjusting the heuristics and making it a little less pessimistic about blacklisting, and it now seems to cause ... few changes to behavior? It's so hard to tell. I can't find a case it's doing worse on yet, anyways. I'll keep looking.
Attachment #374561 - Attachment is obsolete: true
Attachment #374842 - Flags: review?(gal)
Attachment #374561 - Flags: review?(gal)
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Whats the OOM-handling like for the JS hashtable?
Attached patch Update the patch (obsolete) — Splinter Review
This variant is agnostic wrt. errors in the hashtable code. If it runs out of memory or fails to initialize, it just falls back to behaving the way the code currently does (at risk of thereby reverting to "possibly missing heavy-cache-flushing PCs", can't win 'em all!)

Running it through try servers now.
Attachment #374842 - Attachment is obsolete: true
Attachment #376112 - Flags: review?(gal)
Attachment #374842 - Flags: review?(gal)
Attachment #376112 - Flags: review?(gal) → review?(brendan)
Comment on attachment 376112 [details] [diff] [review]
Update the patch

>+typedef struct PCHashEntry {
>+    JSDHashEntryStub stub;
>+    size_t          count;
>+} PCHashEntry;

Super-nit, but since JSDHashEntryStub is p.o.d. you could derive from it rather than embed it. Also, C++ now means no need to typedef to get the struct tag to be a typename.

>+        PCHashEntry *entry = (PCHashEntry *)
>+            JS_DHashTableOperate(table, pc, JS_DHASH_LOOKUP);
>+        
>+        if (entry) {

JS_DHASH_LOOKUP never returns null.

>+            if (JS_DHASH_ENTRY_IS_FREE(&(entry->stub.hdr))) {
>+                entry = (PCHashEntry *)
>+                    JS_DHashTableOperate(table, pc, JS_DHASH_ADD);

Avoid hashing twice by calling JS_DHASH_ADD up front. You can then check for OOM by testing for a null return, and if non-null, JS_DHASH_ENTRY_IS_FREE to claim the added, fresh entry.

>+js_resetRecordingAttempts(JSContext *cx, jsbytecode* pc)
>+{
>+    JSDHashTable *table = &JS_TRACE_MONITOR(cx).recordAttempts;
>+    if (table->ops) {
>+        PCHashEntry *entry = (PCHashEntry *)
>+            JS_DHashTableOperate(table, pc, JS_DHASH_LOOKUP);
>+
>+        if (!entry || JS_DHASH_ENTRY_IS_FREE(&(entry->stub.hdr)))

Again JS_DHASH_LOOKUP means JS_DHashTableOperate never returns null.

Andreas ok'ed my stealing this review.

/be
Attached patch Update the patchSplinter Review
Incorporate brendan's review comments.
Attachment #376112 - Attachment is obsolete: true
Attachment #376146 - Flags: review?(brendan)
Attachment #376112 - Flags: review?(brendan)
Comment on attachment 376146 [details] [diff] [review]
Update the patch

>+    if (table->ops) {
>+        PCHashEntry *entry = (PCHashEntry *)
>+            JS_DHashTableOperate(table, pc, JS_DHASH_ADD);
>+        
>+        if (entry) {
>+            if (!entry->key) {
>+                entry->key = pc;
>+                entry->count = 0;
>+            }

Uber-nit: could just assert entry->count == 0 instead of setting it to 0, eh?

r=me anyway.

/be
Attachment #376146 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/235dfb7751ab

(Using an assert instead, as you suggest.)
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/235dfb7751ab
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.