TM: Need an API exposed to control code cache size

RESOLVED FIXED in mozilla1.9.1

Status

()

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

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: gal)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1
x86
Mac OS X
fixed1.9.1
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)

Updated

9 years ago
Assignee: general → gal
tracking-fennec: --- → ?
Priority: -- → P2

Updated

9 years ago
tracking-fennec: ? → 1.0b1+
(Assignee)

Comment 1

9 years ago
Created attachment 364217 [details] [diff] [review]
patch
(Assignee)

Updated

9 years ago
Attachment #364217 - Flags: review?(danderson)
(Assignee)

Comment 2

9 years ago
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?
(Assignee)

Comment 4

9 years ago
Yeah. Changed to 16*1024*1024 and 1*1024*1024. Thanks.
(Assignee)

Comment 5

9 years ago
Comment on attachment 364217 [details] [diff] [review]
patch

stuart wants this landed tonight if possible
Attachment #364217 - Flags: review?(danderson) → review?(brendan)
(Assignee)

Comment 6

9 years ago
Created attachment 365817 [details] [diff] [review]
v2, with proper default sizes
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
(Assignee)

Comment 9

9 years ago
Yeah, in a sec. Debugging the SetProp assert.
(Assignee)

Comment 10

9 years ago
Created attachment 366084 [details] [diff] [review]
v3
Attachment #365817 - Attachment is obsolete: true
Attachment #365817 - Flags: review?(brendan)
gal, did you mean to re-request review here?
(Assignee)

Updated

9 years ago
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+
(Assignee)

Comment 13

9 years ago
Created attachment 370512 [details] [diff] [review]
v4

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
(Assignee)

Comment 14

9 years ago
Created attachment 370514 [details] [diff] [review]
v5, added an assert
Attachment #370512 - Attachment is obsolete: true
(Assignee)

Comment 15

9 years ago
Created attachment 370517 [details] [diff] [review]
v6, support on-demand flushing of the code cache
Attachment #370514 - Attachment is obsolete: true
(Assignee)

Comment 16

9 years ago
FlushJITCache no longer aborts the recorder. I added asserts to make sure the recorder is already off, or we abort manually right before FlushJITCache.
(Assignee)

Updated

9 years ago
Attachment #370517 - Flags: review?(graydon)
(Assignee)

Updated

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

Comment 19

9 years ago
Created attachment 373894 [details] [diff] [review]
v7, without any changes to the flush mechanism
Attachment #370517 - Attachment is obsolete: true
Attachment #373894 - Flags: review?(vladimir)
Attachment #370517 - Flags: review?(graydon)
(Assignee)

Comment 20

9 years ago
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+
(Assignee)

Comment 22

9 years ago
try server still cycling
Whiteboard: checkin-needed
(Assignee)

Comment 23

9 years ago
Created attachment 373950 [details] [diff] [review]
v8
Attachment #373894 - Attachment is obsolete: true
Attachment #373950 - Flags: review?(mrbkap)
(Assignee)

Updated

9 years ago
Whiteboard: checkin-needed
(Assignee)

Comment 24

9 years ago
Ah crap messed up the patch. I hate this bug.
(Assignee)

Comment 25

9 years ago
Created attachment 373957 [details] [diff] [review]
v9

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)
(Assignee)

Comment 26

9 years ago
Created attachment 373976 [details] [diff] [review]
v10
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+
(Assignee)

Comment 28

9 years ago
Created attachment 374143 [details] [diff] [review]
v10, with 1mb code cache for workers
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

Updated

9 years ago
Attachment #374143 - Flags: superreview?(mrbkap) → superreview+
Attachment #374143 - Flags: review-
(Assignee)

Comment 30

9 years ago
Created attachment 374153 [details] [diff] [review]
v11, with new public API
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.
(Assignee)

Comment 33

9 years ago
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

Comment 36

9 years ago
http://hg.mozilla.org/mozilla-central/rev/e61467ec4978
Status: NEW → RESOLVED
Last Resolved: 9 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.