Closed
Bug 1417380
Opened 7 years ago
Closed 7 years ago
Investigate simplifying finalization phases
Categories
(Core :: JavaScript: GC, enhancement, P3)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: jonco, Assigned: jonco)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
2.18 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
5.52 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
There are several phases for foreground and background object finalization (the data starting with ForegroundObjectFinalizePhase in jsgc.cpp). These were all certainly required at some point, but may not be any more. We should investigate what the dependencies are between the different phases and simplify where possible by merging them.
Updated•7 years ago
|
status-firefox59:
--- → fix-optional
Priority: -- → P3
Updated•7 years ago
|
Assignee: nobody → cduan
Comment 1•7 years ago
|
||
Sorry for the late update. I tried to reduce the finalization works. For example, if we can ensure a JSString won't have a non-inline buffer(a.k.a is inlined) then we don't need the finalizer invocation. Or try to reduce the time iterates over the zone again and again. Some testings are still ongoing but it's almost there. Most of them have no dependency between as a result it will be good chance for parallel these operations if it's worth to. <none>
Attachment #8942803 -
Flags: feedback?(jcoppeard)
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Chia-Hung Duan from comment #1) > For example, if we can ensure a > JSString won't have a non-inline buffer(a.k.a is inlined) then we don't need > the finalizer invocation. That's not the aim of this bug. That may be a valid thing to do, but we should do that somewhere else. > Or try to reduce the time iterates over the zone > again and again. Some testings are still ongoing but it's almost there. Yes, the idea for this bug is to reduce the number of different background finalisation phases and merge these if possible and there are no dependencies. This will reduce the number of times we iterate over the list of zones. > Most of them have no dependency between as a result it will be good chance > for parallel these operations if it's worth to. OK that's a good to know. We definitely want to parallelise this more in the future but we will have to take care we don't create too much lock contention in the memory allocator (jemalloc).
Assignee | ||
Comment 3•7 years ago
|
||
Comment on attachment 8942803 [details] [diff] [review] Minor simplify background finalization. [WIP] Review of attachment 8942803 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I hadn't read the patch when I wrote the last comment. Yes, this is a good thing to do and we can do it in this bug if you like. I didn't realise we called finalisers for some cases where we didn't do anything in the finaliser. This is a good find. Thanks! ::: js/src/jsgc.cpp @@ +3455,5 @@ > } > } > +#endif > + // We could parallelize this in each zone level. > + for (Zone* zone = zones.front(); zone; zone = zone->nextZone()) { Great, good to know we can hoist this loop. ::: js/src/vm/String-inl.h @@ +396,3 @@ > MOZ_ASSERT(getAllocKind() == js::gc::AllocKind::ATOM || > getAllocKind() == js::gc::AllocKind::FAT_INLINE_ATOM); > +#endif Maybe make this assert kind == ATOM only.
Attachment #8942803 -
Flags: feedback?(jcoppeard) → feedback+
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8942803 [details] [diff] [review] Minor simplify background finalization. [WIP] Review of attachment 8942803 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgc.cpp @@ +3469,5 @@ > > AutoLockGC lock(rt); > > // Release swept arenas, dropping and reaquiring the lock every so often to > // avoid blocking the active thread from allocating chunks. Another thing you could try would be to move this block inside the zone loop so we free empty arenas at the end of sweeping every zone. This would free up memory a bit sooner. I think this should work, but I haven't tried it.
Assignee | ||
Comment 5•7 years ago
|
||
I'm going to split this patch up a bit. Here are some improvements to string finalization so that we don't do any work for fat inline strings and atoms where we know from the arena kind that no finalization is required.
Assignee: f103119 → jcoppeard
Attachment #8942803 -
Attachment is obsolete: true
Attachment #8947418 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•7 years ago
|
||
Refactor background sweeping and make it release free arenas sooner.
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8947420 [details] [diff] [review] bug1417380-simplify-background-sweep The main idea here is to make background sweeping sweep arenas by zone, finalization phase and then kind, rather than sweeping by phase, zone and then kind, which doesn't really make sense. Now that zone is the outer loop we can also release free arenas after sweeping every zone which should free up memory sooner. One thing I ran into was that we need to sweep the atoms zone last in case finalizers access atom pointers. Currently this happens because modules hold HeapPtrAtoms which touch their contents in their destructor (this is really a bug too as these should be GCPtrs, see bug 1434882). The atoms zone always ends up last in an incremental GC because we group zones for sweeping and all zones have an edge to the atoms zone, but we skip this for non-incremental GCs.
Attachment #8947420 -
Flags: review?(sphink)
Comment 8•7 years ago
|
||
Comment on attachment 8947418 [details] [diff] [review] bug1417380-improve-string-sweeping Review of attachment 8947418 [details] [diff] [review]: ----------------------------------------------------------------- Sweet. ::: js/src/vm/String-inl.h @@ +395,5 @@ > fop->free_(nonInlineCharsRaw()); > } > > inline void > +js::FatInlineAtom::finalize(js::FreeOp* fop) { Nit: { on its own line
Attachment #8947418 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8947420 -
Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bea7541a608b Avoid calling string finalizers in cases where we know there's no work to do r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/3c74a02c5b6f Make background sweeping sweep by zone and free empty arenas after each zone r=sfink
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bea7541a608b https://hg.mozilla.org/mozilla-central/rev/3c74a02c5b6f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•