Closed Bug 720753 Opened 12 years ago Closed 12 years ago

hoist caches/pools from JSCompartment into JSRuntime

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(7 files, 2 obsolete files)

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).
Attached patch hoist scriptFilenameTable (obsolete) — Splinter Review
Attachment #591162 - Flags: review?(igor)
Flagging bhackett for overall patch, njn for the memory metrics changes.
Attachment #591163 - Flags: review?(n.nethercote)
Attachment #591163 - Flags: review?(bhackett1024)
Attachment #591164 - Flags: review?(bhackett1024)
Attachment #591165 - Flags: review?(bhackett1024)
Here, I changed ToSourceCache a bit to resemble the other caches.
Attachment #591166 - Flags: review?(christopher.leary)
Attached patch hoist evalCache (obsolete) — Splinter Review
Attachment #591167 - Flags: review?(igor)
Igor pointed out on irc that the previous patch would mark ScriptFilenameEntry during compartmental gc which would unnecessarily extend their lifetime.
Attachment #591162 - Attachment is obsolete: true
Attachment #591162 - Flags: review?(igor)
Attachment #591277 - Flags: review?(igor)
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.
Attachment #591167 - Flags: review?(igor)
Comment on attachment 591277 [details] [diff] [review]
hoist stringFilenameTable v.2

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

nice!
Attachment #591277 - Flags: review?(igor) → review+
Whiteboard: [MemShrink]
Attachment #591167 - Attachment is obsolete: true
Attachment #591330 - Flags: review?(igor)
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.
Attachment #591163 - Flags: review?(bhackett1024) → review+
Attachment #591164 - Flags: review?(bhackett1024) → review+
Attachment #591165 - Flags: review?(bhackett1024) → review+
Attachment #591330 - Flags: review?(igor) → review+
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.
Attachment #591163 - Flags: review?(n.nethercote) → review+
(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?
(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.
Blocks: cpg
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.
Attachment #591166 - Flags: review?(christopher.leary) → review+
Blocks: 722775
Whiteboard: [MemShrink] → [MemShrink:P1]
Almost time to drop these bombs.
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?
I dunno, I just mv'd the code.  What does hg annotate say?
This regressed our Trace Malloc MaxHeap measurement by 42%.
Did this show up just now, 1 month later?  Note also these csets landed next to cpg.
No, it showed up on Talos back then.  I just noticed in the series of emails caused by pushing 15 to Aurora.
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.
Aha, the windows build on the CPG push failed, so there's no data point for that.

CPG is probably to blame then.
You need to log in before you can comment on or make changes to this bug.