Closed
Bug 720753
Opened 12 years ago
Closed 12 years ago
hoist caches/pools from JSCompartment into JSRuntime
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: luke, Assigned: luke)
References
Details
(Whiteboard: [MemShrink:P1])
Attachments
(7 files, 2 obsolete files)
58.00 KB,
patch
|
bhackett1024
:
review+
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
22.88 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
9.14 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
6.21 KB,
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
11.30 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
8.14 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
104.26 KB,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Attachment #591162 -
Flags: review?(igor)
Assignee | ||
Comment 2•12 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•12 years ago
|
||
Attachment #591164 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #591165 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 5•12 years ago
|
||
Here, I changed ToSourceCache a bit to resemble the other caches.
Attachment #591166 -
Flags: review?(christopher.leary)
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #591167 -
Flags: review?(igor)
Assignee | ||
Comment 7•12 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•12 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•12 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+
Updated•12 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #591167 -
Attachment is obsolete: true
Attachment #591330 -
Flags: review?(igor)
Comment 11•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #591164 -
Flags: review?(bhackett1024) → review+
Updated•12 years ago
|
Attachment #591165 -
Flags: review?(bhackett1024) → review+
Updated•12 years ago
|
Attachment #591330 -
Flags: review?(igor) → review+
Comment 12•12 years ago
|
||
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•12 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•12 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.
Comment 15•12 years ago
|
||
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+
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Assignee | ||
Comment 16•12 years ago
|
||
Almost time to drop these bombs.
Comment 17•12 years ago
|
||
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/400c2b30015d http://hg.mozilla.org/integration/mozilla-inbound/rev/9148e0a14036 http://hg.mozilla.org/integration/mozilla-inbound/rev/8d2ae5e00295 http://hg.mozilla.org/integration/mozilla-inbound/rev/6005ea04a5c7 http://hg.mozilla.org/integration/mozilla-inbound/rev/f4dc271213a4 http://hg.mozilla.org/integration/mozilla-inbound/rev/78445ab314a4
Target Milestone: --- → mozilla15
Comment 18•12 years ago
|
||
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•12 years ago
|
||
I dunno, I just mv'd the code. What does hg annotate say?
Comment 20•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/78445ab314a4 http://hg.mozilla.org/mozilla-central/rev/f4dc271213a4 http://hg.mozilla.org/mozilla-central/rev/6005ea04a5c7 http://hg.mozilla.org/mozilla-central/rev/8d2ae5e00295 http://hg.mozilla.org/mozilla-central/rev/9148e0a14036 http://hg.mozilla.org/mozilla-central/rev/400c2b30015d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
This regressed our Trace Malloc MaxHeap measurement by 42%.
Assignee | ||
Comment 22•12 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•12 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.
Description
•