TM: Need an API exposed to control code cache size

RESOLVED FIXED in mozilla1.9.1

Status

()

defect
P2
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: bent.mozilla, Assigned: gal)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
wanted1.9.1 +

Firefox Tracking Flags

(fennec1.0b1+)

Details

Attachments

(1 attachment, 11 obsolete attachments)

Workers, for instance, may need to limit the size of the code cache so that we don't eat through too much memory.
Assignee: general → gal
tracking-fennec: --- → ?
Priority: -- → P2
tracking-fennec: ? → 1.0b1+
Posted patch patch (obsolete) — Splinter Review
Attachment #364217 - Flags: review?(danderson)
As per sayrer patch will not go in until after b3 freeze.
Comment on attachment 364217 [details] [diff] [review]
patch

>+    fragmento->setLimit(24);
...
>+    fragmento->setLimit(20);

Do those need to be in bytes now?
Yeah. Changed to 16*1024*1024 and 1*1024*1024. Thanks.
Comment on attachment 364217 [details] [diff] [review]
patch

stuart wants this landed tonight if possible
Attachment #364217 - Flags: review?(danderson) → review?(brendan)
Posted patch v2, with proper default sizes (obsolete) — Splinter Review
Attachment #364217 - Attachment is obsolete: true
Attachment #365817 - Flags: review?(brendan)
Attachment #364217 - Flags: review?(brendan)
Comment on attachment 365817 [details] [diff] [review]
v2, with proper default sizes

> extern void
>+js_SetMaxCodeCacheSize(JSRuntime* rt, uint32 bytes)
>+{

extern on definition is deadwood given .h file extern prototype declaration; there seem to be five in tip tm jstracer.cpp at these lines:

$ grep -n '^extern ' jstracer.cpp
4490:extern void
4528:extern void
4601:extern void
4609:extern JS_REQUIRES_STACK void
4635:extern JS_REQUIRES_STACK void

Could you axe 'em?

>+#ifdef JS_THREADSAFE
>+    JSTraceMonitor* tm = &js_GetCurrentThread(rt)->traceMonitor;
>+#else
>+    JSTraceMonitor* tm = &rt->traceMonitor;
>+#endif
>+    JS_ASSERT(tm->fragmento && tm->reFragmento);
>+
>+    tm->fragmento->setLimit(bytes);
>+    tm->reFragmento->setLimit(bytes/16); // 1/16th of the size of regexp JIT

Comment should say "regexp JIT uses 1/16th the size", but the code says that. Really, comment should say why this ratio is used, or have a "FIXME: tune this using bug NNNNNN" comment.

We use spaces around binary operators, usually.

> #if !defined XP_WIN

Why is this #ifndef XP_WIN needed? It's pre-existing but I forget.

>+    void Fragmento::setLimit(uint32 bytes)
>+    {
>+        _max_pages = 4;
>+        while (uint32_t(NJ_PAGE_SIZE * _max_pages) < bytes &&
>+               _max_pages < (1<<30))
>+            ++_max_pages;

This should be

        _max_pages = JS_MIN(JS_HOWMANY(bytes, NJ_PAGE_SIZE), JS_BIT(30));

or equivalent. Guess you can't use jstypes.h macros in nanojit, but these are short enough you could expand them, optimize a bit to do explicit CSE.

There's no cause for a loop that counts pages one by one though.

>+        JS_SetGCParameter(mJSRuntime, JSGC_MAX_CODE_CACHE_BYTES, 16 * 1024 * 1024);

Do we want to go to 32 here, or separate bug?

/be
New patch coming?

/be
Yeah, in a sec. Debugging the SetProp assert.
Posted patch v3 (obsolete) — Splinter Review
Attachment #365817 - Attachment is obsolete: true
Attachment #365817 - Flags: review?(brendan)
gal, did you mean to re-request review here?
Attachment #366084 - Flags: review?(mrbkap)
Comment on attachment 366084 [details] [diff] [review]
v3

>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
>+    /* Size of the code cache in pages. */
>+    JSGC_MAX_CODE_CACHE_BYTES = 6

The comment should also say "bytes" right?
Attachment #366084 - Flags: review?(mrbkap) → review+
Posted patch v4 (obsolete) — Splinter Review
This patch also avoids flushing if we have native code on the stack and can't safely do that.
Attachment #366084 - Attachment is obsolete: true
Posted patch v5, added an assert (obsolete) — Splinter Review
Attachment #370512 - Attachment is obsolete: true
Attachment #370514 - Attachment is obsolete: true
FlushJITCache no longer aborts the recorder. I added asserts to make sure the recorder is already off, or we abort manually right before FlushJITCache.
Attachment #370517 - Flags: review?(graydon)
Attachment #370517 - Flags: review?(jorendorff)
Generally happy with the approach. A little unclear about some details.

- The assert on line 3014, in js_DeleteRecorder, is redundant. You NULL'ed
  out the recorder a few lines before.

- Similarly, the asserts in js_RecordTree on lines 3309, 3322 and 3331 are
  redundant. You checked the recorder on entry to the function.

- I'm not seeing the motivation for moving the conditional-abort out of
  js_FlushJITCache. If you prefer it this way ... I guess I don't mind, but
  it seems identical to me. Is this intentional?

- The constant '16' on line 4821 should be named. RE_FRAGMENTO_RATIO or 
  something. It's arbitrary, but it should be clearly noted as such.

- You do prohibitCodeCacheFlush++ in js_DeepBail, but seemingly never do
  prohibitCodeCacheFlush--. It seems once you've prohibited it, you've
  done so for good. Is this intentional? I imagine not, but can't tell.
Comment on attachment 370517 [details] [diff] [review]
v6, support on-demand flushing of the code cache

This patch needs to be rebased.
Attachment #370517 - Flags: review?(jorendorff)
Attachment #370517 - Attachment is obsolete: true
Attachment #373894 - Flags: review?(vladimir)
Attachment #370517 - Flags: review?(graydon)
Blocking fennec. Want to get this out of the way. Try server run is on the way.
Flags: wanted1.9.1?
Comment on attachment 373894 [details] [diff] [review]
v7, without any changes to the flush mechanism

Looks fine to me.
Attachment #373894 - Flags: review?(vladimir) → review+
try server still cycling
Whiteboard: checkin-needed
Posted patch v8 (obsolete) — Splinter Review
Attachment #373894 - Attachment is obsolete: true
Attachment #373950 - Flags: review?(mrbkap)
Whiteboard: checkin-needed
Ah crap messed up the patch. I hate this bug.
Posted patch v9 (obsolete) — Splinter Review
Handing off to bent to figure out how to wire this up to the browser.
Attachment #373950 - Attachment is obsolete: true
Attachment #373950 - Flags: review?(mrbkap)
Posted patch v10 (obsolete) — Splinter Review
Attachment #373957 - Attachment is obsolete: true
Attachment #373976 - Flags: review?(bent.mozilla)
Comment on attachment 373976 [details] [diff] [review]
v10

>diff --git a/dom/src/threads/nsDOMThreadService.cpp
> ...
>+  /*
>+   * This cannot be done until we have associated the current thread with the runtime,
>+   * which happens when we create the first context on it.
>+   */
>+  JS_SetGCParameter(rt, JSGC_MAX_CODE_CACHE_BYTES, 16 * 1024 * 1024);

For workers we only want 1meg cache, so take out the 16*, and the comment doesn't really make sense so I'd just remove it. Sorry, should have seen that earlier. Everything else looks great!
Attachment #373976 - Flags: review?(bent.mozilla) → review+
Attachment #374143 - Flags: superreview?(mrbkap)
Instead of a runtime-parameterized API that targets the current thread, why not add a JS_SetGCParameterForThread(cx)? No need to expose JSThread, the cx is the right param.

/be
Attachment #374143 - Flags: superreview?(mrbkap) → superreview+
Attachment #374143 - Flags: review-
Attachment #373976 - Attachment is obsolete: true
Attachment #374143 - Attachment is obsolete: true
Attachment #374153 - Flags: review?(brendan)
Comment on attachment 374153 [details] [diff] [review]
v11, with new public API

>+  JS_SetGCParameterForThread(cx, JSGC_MAX_CODE_CACHE_BYTES, 1 * 1024 * 1024);

I missed the background -- is 1MB enough? Too much? Any data informing this choice?

#define K *1024
#define M K K
#define G K M

would come in handy throughout this patch: 1 M, 1 G, etc. Or KB, MB, GB if two letters are needed.

>+++ b/js/src/jscntxt.cpp
>@@ -252,17 +252,16 @@ thread_purger(JSDHashTable *table, JSDHa
>         JS_ASSERT(cx->thread != thread);
>         js_DestroyScriptsToGC(cx, &thread->data);
>         DestroyThread(thread);
>         return JS_DHASH_REMOVE;
>     }
>     PurgeThreadData(cx, &thread->data);
>     return JS_DHASH_NEXT;
> }
>-
> #endif /* JS_THREADSAFE */

Do not make gratuitous and unbalanced blank line changes -- the #ifdef JS_THREADSAFE matching this #endif has a blank line after it -- this is intentional.

>@@ -149,16 +149,22 @@ struct JSTraceMonitor {
>      * If nonzero, do not flush the JIT cache after a deep bail.  That would
>      * free JITted code pages that we will later return to.  Instead, set
>      * the needFlush flag so that it can be flushed later.
>      */
>     uintN                   prohibitFlush;
>     JSPackedBool            needFlush;
> 
>     /*
>+     * Maximum size of the code cache before we start flushing. 1/16 of this
>+     * size is used as threshold for the regular expression code cache.
>+     */
>+    jsuint                  maxCodeCacheBytes;

jsuint is old ECMA veneer to rename uint32. For members of structs in jscntxt.h and elsewhere, use uint32 directly, do not use jsuint.

>@@ -3208,17 +3205,17 @@ js_DeleteRecorder(JSContext* cx)
>     /* Aborting and completing a trace end up here. */
>     delete tm->recorder;
>     tm->recorder = NULL;
> 
>     /*
>      * If we ran out of memory, flush the code cache.
>      */
>     if (JS_TRACE_MONITOR(cx).fragmento->assm()->error() == OutOMem
>-        || js_OverfullFragmento(tm->fragmento, MAX_MEM_IN_MAIN_FRAGMENTO)) {
>+        || js_OverfullFragmento(tm, tm->fragmento)) {

While you are here, please put || at the end of the previous line.

> void
>+js_SetMaxCodeCacheBytes(JSContext* cx, uint32 bytes)
>+{
>+    JSTraceMonitor* tm = &JS_THREAD_DATA(cx)->traceMonitor;
>+    JS_ASSERT(tm->fragmento && tm->reFragmento);
>+    if (bytes > 1024*1024*1024)
>+        bytes = 1024*1024*1024;
>+    if (bytes < 128*1024)
>+        bytes = 128*1024;

More K and G fodder.

>+    tm->maxCodeCacheBytes = 16*1024*1024;

And M fodder.

>@@ -5018,17 +5031,24 @@ js_OverfullFragmento(Fragmento *frago, s
>      * time being: condition 1 is handled by the outOMem flag inside
>      * nanojit, and condition 2 is being handled independently *here*. So
>      * we construct our fragmentos to use all available memory they like,
>      * and only report outOMem to us when there is literally no OS memory
>      * left. Merely purging our cache when we hit our highwater mark is
>      * handled by the (few) callers of this function.
>      *
>      */
>-    return (frago->_stats.pages > (maxsz >> NJ_LOG2_PAGE_SIZE));
>+    jsuint maxsz = tm->maxCodeCacheBytes;
>+    if (fragmento == tm->fragmento) {
>+        if (tm->prohibitFlush)
>+            return false;
>+    } else {
>+        maxsz /= 16;

I remember the magic 16 but not why. Comment relating this to the default size and giving a rationale?

>+    JS_SetGCParameterForThread(cx, JSGC_MAX_CODE_CACHE_BYTES, 16 * 1024 * 1024);

16 M -- I know, C pre-processor hack-a-rama, but it reads better. A JS_M(16) macro does not.

Apart from the K/M/G fun, which is up to you, r=me with fixes to the above.

/be
Attachment #374153 - Flags: review?(brendan) → review+
At the least use |static const size_t KB = 1024, MB = 1024 * KB, GB = 1024 * MB;| if you really must, but I don't think the abbreviation really helps enough to be worthwhile.
Landed into a deeply red tree.

http://hg.mozilla.org/tracemonkey/rev/e61467ec4978
(In reply to comment #32)
> At the least use |static const size_t KB = 1024, MB = 1024 * KB, GB = 1024 *
> MB;| if you really must, but I don't think the abbreviation really helps enough
> to be worthwhile.

Love CPP -- don't hate, love!

The new code is short and to the point. C++ prolixity such as 'static const size_t ...' is an argument for the patch that landed. Long live CPP macros! :-P

/be
http://hg.mozilla.org/mozilla-central/rev/e61467ec4978
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: wanted1.9.1? → wanted1.9.1+
Resolution: --- → FIXED
Sayrer, this bug blocks (or fixes, really) 1.9.1 blocker bug 483915. Marking this a blocker. Can you merge this over to 1.9.1?
Flags: blocking1.9.1+
Target Milestone: --- → mozilla1.9.1
You need to log in before you can comment on or make changes to this bug.