Last Comment Bug 720753 - hoist caches/pools from JSCompartment into JSRuntime
: hoist caches/pools from JSCompartment into JSRuntime
Status: RESOLVED FIXED
[MemShrink:P1]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 675078
Blocks: cpg 722775
  Show dependency treegraph
 
Reported: 2012-01-24 10:25 PST by Luke Wagner [:luke]
Modified: 2012-06-04 21:05 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
hoist scriptFilenameTable (7.84 KB, patch)
2012-01-24 10:30 PST, Luke Wagner [:luke]
no flags Details | Diff | Review
hoist compartment jaegerCompartment (now jaegerRuntime) (58.00 KB, patch)
2012-01-24 10:32 PST, Luke Wagner [:luke]
bhackett1024: review+
n.nethercote: review+
Details | Diff | Review
hoist newObjectCache (22.88 KB, patch)
2012-01-24 10:33 PST, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Review
hoist nativeIterCache (9.14 KB, patch)
2012-01-24 10:33 PST, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Review
hoist toSourceCache (6.21 KB, patch)
2012-01-24 10:35 PST, Luke Wagner [:luke]
cdleary: review+
Details | Diff | Review
hoist evalCache (7.79 KB, patch)
2012-01-24 10:36 PST, Luke Wagner [:luke]
no flags Details | Diff | Review
hoist stringFilenameTable v.2 (11.30 KB, patch)
2012-01-24 14:41 PST, Luke Wagner [:luke]
igor: review+
Details | Diff | Review
hoist evalCache v.2 (8.14 KB, patch)
2012-01-24 17:01 PST, Luke Wagner [:luke]
igor: review+
Details | Diff | Review
combined patch (applies to cset 0a796d07499a) (104.26 KB, patch)
2012-04-30 16:36 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review

Description Luke Wagner [:luke] 2012-01-24 10:25:51 PST
With single-threaded runtime, we can hoist a lot of caches that were only stored per-compartment because compartment was single-threaded.  While the caches do point at compartment-local GC-things,  there isn't a problem as long as the key/lookup value is compartment-local and the cache is purged on every (compartment or full) GC.

The attached set of patches hoist all the caches/pools, leaving only the TI and GC data and random twiddly state.  This takes sizeof(JSCompartment) from 11.1KB to 1.2KB and decreases membuster total explicit allocation (after "Minimize Memory Usage") from 2040MB to 1942MB (a 4.8% improvement).
Comment 1 Luke Wagner [:luke] 2012-01-24 10:30:58 PST
Created attachment 591162 [details] [diff] [review]
hoist scriptFilenameTable
Comment 2 Luke Wagner [:luke] 2012-01-24 10:32:32 PST
Created attachment 591163 [details] [diff] [review]
hoist compartment jaegerCompartment (now jaegerRuntime)

Flagging bhackett for overall patch, njn for the memory metrics changes.
Comment 3 Luke Wagner [:luke] 2012-01-24 10:33:09 PST
Created attachment 591164 [details] [diff] [review]
hoist newObjectCache
Comment 4 Luke Wagner [:luke] 2012-01-24 10:33:58 PST
Created attachment 591165 [details] [diff] [review]
hoist nativeIterCache
Comment 5 Luke Wagner [:luke] 2012-01-24 10:35:45 PST
Created attachment 591166 [details] [diff] [review]
hoist toSourceCache

Here, I changed ToSourceCache a bit to resemble the other caches.
Comment 6 Luke Wagner [:luke] 2012-01-24 10:36:28 PST
Created attachment 591167 [details] [diff] [review]
hoist evalCache
Comment 7 Luke Wagner [:luke] 2012-01-24 14:41:02 PST
Created attachment 591277 [details] [diff] [review]
hoist stringFilenameTable v.2

Igor pointed out on irc that the previous patch would mark ScriptFilenameEntry during compartmental gc which would unnecessarily extend their lifetime.
Comment 8 Igor Bukanov 2012-01-24 14:46:58 PST
Comment on attachment 591167 [details] [diff] [review]
hoist evalCache

Review of attachment 591167 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +967,5 @@
> +     * Clear the hash and reset all evalHashLink to null before the GC. This
> +     * way MarkChildren(trc, JSScript *) can assume that JSScript::u.object is
> +     * not null when we have script owned by an object and not from the eval
> +     * cache.
> +     */

Replace stalled comments with the following text:

Clear the hash and reset all evalHashLink to null before the GC. This way the GC marking code can assume that if a non-null value is stored in the union field { globalObject, evalHashLink } of the script, then that value denotes the global object.

@@ +971,5 @@
> +     */
> +    for (size_t i = 0; i < ArrayLength(table_); ++i) {
> +        for (JSScript **listHeadp = &table_[i]; *listHeadp; ) {
> +            JSScript *script = *listHeadp;
> +            JS_ASSERT(GetGCThingTraceKind(script) == JSTRACE_SCRIPT);

The eval cache is valuable and purging all its entries just because some compartment wants the full GC could be wasteful. So remove the entry only if we have a full GC or when scripp->compartment() matches the GC compartment.
Comment 9 Igor Bukanov 2012-01-24 15:00:55 PST
Comment on attachment 591277 [details] [diff] [review]
hoist stringFilenameTable v.2

Review of attachment 591277 [details] [diff] [review]:
-----------------------------------------------------------------

nice!
Comment 10 Luke Wagner [:luke] 2012-01-24 17:01:30 PST
Created attachment 591330 [details] [diff] [review]
hoist evalCache v.2
Comment 11 Brian Hackett (:bhackett) 2012-01-24 18:13:35 PST
Comment on attachment 591163 [details] [diff] [review]
hoist compartment jaegerCompartment (now jaegerRuntime)

Review of attachment 591163 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/methodjit/Retcon.cpp
@@ +328,2 @@
>           f != NULL;
>           f = f->previous) {

ExpandInlineFrames should only modify frames from the given compartment.  Can you modify this walk to filter the VMFrame's compartment (f->entryfp->callee().compartment() or somesuch should work).

@@ +366,2 @@
>           f != NULL;
>           f = f->previous) {

Ditto.

@@ +426,2 @@
>           f != NULL;
>           f = f->previous) {

Ditto.
Comment 12 Nicholas Nethercote [:njn] 2012-01-25 03:14:16 PST
Comment on attachment 591163 [details] [diff] [review]
hoist compartment jaegerCompartment (now jaegerRuntime)

Review of attachment 591163 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/MemoryMetrics.h
@@ -113,2 @@
>      int64_t mjitData;
> -#endif

You've removed a bunch of |#ifdef JS_METHODJIT|s.  Is this because that #define is disappearing, or did you just decide it wasn't necessary to remove these fields in non-methodjit builds?

::: js/src/jscntxt.cpp
@@ +111,5 @@
>  
> +    if (execAlloc_)
> +        execAlloc_->sizeOfCode(mjitCode, regexpCode, unusedCodeMemory);
> +    else
> +        *mjitCode = *regexpCode = *unusedCodeMemory = 0;

So mjit code and regexp code are now allocated in the same pool?  Probably not a bad thing, since each pool is at least 64KB.

::: js/src/shell/js.cpp
@@ -4041,2 @@
>      JS_FN("mjitdatastats",  MJitDataStats,  0,0),
>  #endif

Heh, in https://hg.mozilla.org/integration/mozilla-inbound/rev/a6849eb97d82 I removed |mjitdatastats|!

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1653,5 @@
>                         callback, closure);
>  
> +    ReportMemoryBytes0(pathPrefix + NS_LITERAL_CSTRING("runtime/unused-code-memory"),
> +                       nsIMemoryReporter::KIND_NONHEAP, data.runtimeUnusedCodeMemory,
> +                       "Memory lost due to fragmentation in the runtime's ExecutableAllocator.",

It's not lost necessarily lost to fragmentation.  When you first allocate a 64KB code chunk it's initially all unused, and then you gradually fill the chunk up.

We used to have something very similar per-compartment, viz:

├──10,813,440 B (01.36%) -- mjit-code
│  ├──10,415,160 B (01.31%) -- method
│  ├─────333,920 B (00.04%) -- regexp
│  └──────64,360 B (00.01%) -- unused

The description for "unused" was "Memory allocated by the method and/or regexp JIT to hold the compartment's code, but which is currently unused."  I'd change this description to something like that.
Comment 13 Luke Wagner [:luke] 2012-01-25 10:51:55 PST
(In reply to Brian Hackett (:bhackett) from comment #11)
> ExpandInlineFrames should only modify frames from the given compartment. 
> Can you modify this walk to filter the VMFrame's compartment
> (f->entryfp->callee().compartment() or somesuch should work).

Will do (fp->compartment() :).  A question: would it have been correct (just suboptimal) to throw away all compartments' frames?
Comment 14 Luke Wagner [:luke] 2012-01-25 10:58:27 PST
(In reply to Nicholas Nethercote [:njn] from comment #12)
> You've removed a bunch of |#ifdef JS_METHODJIT|s.  Is this because that
> #define is disappearing, or did you just decide it wasn't necessary to
> remove these fields in non-methodjit builds?

I only removed the JS_METHODJIT #ifdefs for the memory accounting since less #ifdefs a happier developer make.  However I tested that it is still possible to build and run a JS shell with --disable-methodjit.

> So mjit code and regexp code are now allocated in the same pool?  Probably
> not a bad thing, since each pool is at least 64KB.

Correct.
Comment 15 Chris Leary [:cdleary] (not checking bugmail) 2012-01-25 22:43:44 PST
Comment on attachment 591166 [details] [diff] [review]
hoist toSourceCache

Review of attachment 591166 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfun.cpp
@@ +1515,5 @@
> +            return;
> +        map_->init();
> +    }
> +
> +    (void) map_->put(fun, str);

TIL js::HashTable remains in a valid state in the case of add() failure.
Comment 16 Luke Wagner [:luke] 2012-04-30 16:36:36 PDT
Created attachment 619754 [details] [diff] [review]
combined patch (applies to cset 0a796d07499a)

Almost time to drop these bombs.
Comment 18 :Ms2ger 2012-05-03 01:12:07 PDT
Comment on attachment 591164 [details] [diff] [review]
hoist newObjectCache

Review of attachment 591164 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jscntxtinlines.h
@@ +120,5 @@
> +
> +inline void
> +NewObjectCache::fillGlobal(EntryIndex entry, Class *clasp, js::GlobalObject *global, gc::AllocKind kind, JSObject *obj)
> +{
> +    //JS_ASSERT(global == obj->getGlobal());

Do we have a bug to enable this assertion?
Comment 19 Luke Wagner [:luke] 2012-05-03 01:17:05 PDT
I dunno, I just mv'd the code.  What does hg annotate say?
Comment 21 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-06-04 19:08:12 PDT
This regressed our Trace Malloc MaxHeap measurement by 42%.
Comment 22 Luke Wagner [:luke] 2012-06-04 20:35:05 PDT
Did this show up just now, 1 month later?  Note also these csets landed next to cpg.
Comment 23 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-06-04 20:37:53 PDT
No, it showed up on Talos back then.  I just noticed in the series of emails caused by pushing 15 to Aurora.
Comment 24 Luke Wagner [:luke] 2012-06-04 20:57:48 PDT
Ah.  I still suspect you are confusing this bug with cpg which immediately precedes it in the changelog.  Here is the talos comparison for the patches in this bug:
  http://perf.snarkfest.net/compare-talos/index.html?oldRevs=bed8c4e3dfdf&newRev=400c2b30015d&submit=true
That's a lot of green.
Comment 25 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-06-04 21:05:20 PDT
Aha, the windows build on the CPG push failed, so there's no data point for that.

CPG is probably to blame then.

Note You need to log in before you can comment on or make changes to this bug.