TM: Need an API exposed to control code cache size

RESOLVED FIXED in mozilla1.9.1

Status

()

P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: bent.mozilla, Assigned: gal)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1
x86
macOS
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

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

Updated

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

Comment 1

10 years ago
Posted patch patch (obsolete) — Splinter Review
(Assignee)

Updated

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

Comment 2

10 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

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

Comment 5

10 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

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

Comment 9

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

Comment 10

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

Updated

10 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

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

Comment 14

10 years ago
Posted patch v5, added an assert (obsolete) — Splinter Review
Attachment #370512 - Attachment is obsolete: true
(Assignee)

Comment 15

10 years ago
Attachment #370514 - Attachment is obsolete: true
(Assignee)

Comment 16

10 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

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

Updated

10 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

10 years ago
Attachment #370517 - Attachment is obsolete: true
Attachment #373894 - Flags: review?(vladimir)
Attachment #370517 - Flags: review?(graydon)
(Assignee)

Comment 20

10 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

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

Comment 23

10 years ago
Posted patch v8 (obsolete) — Splinter Review
Attachment #373894 - Attachment is obsolete: true
Attachment #373950 - Flags: review?(mrbkap)
(Assignee)

Updated

10 years ago
Whiteboard: checkin-needed
(Assignee)

Comment 24

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

Comment 25

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

Comment 26

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

Comment 28

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

Comment 30

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

10 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

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