474497 - TM: Need an API exposed to control code cache size

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Workers, for instance, may need to limit the size of the code cache so that we don't eat through too much memory.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

As per sayrer patch will not go in until after b3 freeze.

Comment 3

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

Comment on attachment 364217 [details] [diff] [review]
patch

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

Do those need to be in bytes now?

Comment 4

[Andreas Gal :gal]

Yeah. Changed to 16*1024*1024 and 1*1024*1024. Thanks.

Comment 5

[Andreas Gal :gal]

Comment on attachment 364217 [details] [diff] [review]
patch

stuart wants this landed tonight if possible

Comment 6

[Andreas Gal :gal]

Attachment: v2, with proper default sizes

Comment 7

[Brendan Eich [: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

Comment 8

[Brendan Eich [:brendan]]

New patch coming?

/be

Comment 9

[Andreas Gal :gal]

Yeah, in a sec. Debugging the SetProp assert.

Comment 10

[Andreas Gal :gal]

Attachment: v3

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

gal, did you mean to re-request review here?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 13

[Andreas Gal :gal]

Attachment: v4

This patch also avoids flushing if we have native code on the stack and can't safely do that.

Comment 14

[Andreas Gal :gal]

Attachment: v5, added an assert

Comment 15

[Andreas Gal :gal]

Attachment: v6, support on-demand flushing of the code cache

Comment 16

[Andreas Gal :gal]

FlushJITCache no longer aborts the recorder. I added asserts to make sure the recorder is already off, or we abort manually right before FlushJITCache.

Comment 17

[Graydon Hoare :graydon]

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 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 370517 [details] [diff] [review]
v6, support on-demand flushing of the code cache

This patch needs to be rebased.

Comment 19

[Andreas Gal :gal]

Attachment: v7, without any changes to the flush mechanism

Comment 20

[Andreas Gal :gal]

Blocking fennec. Want to get this out of the way. Try server run is on the way.

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 373894 [details] [diff] [review]
v7, without any changes to the flush mechanism

Looks fine to me.

Comment 22

[Andreas Gal :gal]

try server still cycling

Comment 23

[Andreas Gal :gal]

Attachment: v8

Comment 24

[Andreas Gal :gal]

Ah **** messed up the patch. I hate this bug.

Comment 25

[Andreas Gal :gal]

Attachment: v9

Handing off to bent to figure out how to wire this up to the browser.

Comment 26

[Andreas Gal :gal]

Attachment: v10

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!

Comment 28

[Andreas Gal :gal]

Attachment: v10, with 1mb code cache for workers

Comment 29

[Brendan Eich [:brendan]]

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

Comment 30

[Andreas Gal :gal]

Attachment: v11, with new public API

Comment 31

[Brendan Eich [: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

Comment 32

[Jeff Walden [:Waldo]]

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.

Comment 33

[Andreas Gal :gal]

Landed into a deeply red tree.

http://hg.mozilla.org/tracemonkey/rev/e61467ec4978

Comment 34

[Andreas Gal :gal]

Oops. http://hg.mozilla.org/tracemonkey/rev/3a7d7f250764

Comment 35

[Brendan Eich [:brendan]]

(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

Comment 36

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e61467ec4978

Comment 37

[Johnny Stenback (:jst)]

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?

Comment 38

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/baae8ad4fcb2