hoist caches/pools from JSCompartment into JSRuntime

RESOLVED FIXED in mozilla15

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(7 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

7 years ago
Posted patch hoist scriptFilenameTable (obsolete) — Splinter Review
Attachment #591162 - Flags: review?(igor)
(Assignee)

Comment 2

7 years ago
Flagging bhackett for overall patch, njn for the memory metrics changes.
Attachment #591163 - Flags: review?(n.nethercote)
Attachment #591163 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

7 years ago
Attachment #591164 - Flags: review?(bhackett1024)
(Assignee)

Comment 4

7 years ago
Attachment #591165 - Flags: review?(bhackett1024)
(Assignee)

Comment 5

7 years ago
Here, I changed ToSourceCache a bit to resemble the other caches.
Attachment #591166 - Flags: review?(christopher.leary)
(Assignee)

Comment 6

7 years ago
Posted patch hoist evalCache (obsolete) — Splinter Review
Attachment #591167 - Flags: review?(igor)
(Assignee)

Comment 7

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

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

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

Comment 10

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

Updated

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

Comment 13

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

Comment 14

7 years ago
(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.

Updated

7 years ago
Blocks: 650353
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+
(Assignee)

Updated

7 years ago
Blocks: 722775
Whiteboard: [MemShrink] → [MemShrink:P1]
(Assignee)

Comment 16

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

Comment 19

7 years ago
I dunno, I just mv'd the code.  What does hg annotate say?
This regressed our Trace Malloc MaxHeap measurement by 42%.
(Assignee)

Comment 22

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

Comment 24

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