Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Description • 7 years ago

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.

	Chia-Hung Duan
	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>

	Jon Coppeard (:jonco) (PTO until 14th September) 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).

	Jon Coppeard (:jonco) (PTO until 14th September) 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.

	Jon Coppeard (:jonco) (PTO until 14th September) 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.

	Jon Coppeard (:jonco) (PTO until 14th September) 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.

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 6 • 7 years ago

Refactor background sweeping and make it release free arenas sooner.

	Jon Coppeard (:jonco) (PTO until 14th September) 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.

	Jan de Mooij [:jandem]
	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

	Pulsebot
	Comment 9 • 7 years ago

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

	Raul Gurzau (:RaulG)
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bea7541a608b
https://hg.mozilla.org/mozilla-central/rev/3c74a02c5b6f