1118481 - Simplify GC allocation logic somewhat

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

In bug 1005849 I added extra logic to make the GC's allocator do a better job of finding aligned chunks high fragmentation / low memory situations. Afterward, I proposed the same thing for bug 1005844, and that ended up going through several more iterations that significantly simplified the logic. Though that patch may never land, we can still apply these simplifications to the GC allocation logic.

:erahm mentioned on IRC that the current code makes Coverity upset, so it's probably a good idea to fix it now.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Simplify the GC allocation logic using the implementation from bug 1005844.

So this ports over the approach from bug 1005844, including some of the comments. I don't know if those are strictly better than what we had before, but they did go through a few more iterations.

In addition, this changes growthDirection to a mozilla::Atomic with relaxed memory ordering. We used to have a growthDirection per SystemPageAllocator, but now that the allocation logic is shared across threads we should take care not to mess up this value. The memory ordering doesn't matter, since even if one thread sees an out-of-date value and assumes the wrong direction, it will just cause a superfluous system call that one time.

I've confirmed that the jsapi-test still passes with this patch applied locally on Windows and Linux. Try at: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ef93e419d883

Eric, could you see if this fixes the Coverity issue you saw?

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8544838 [details] [diff] [review]
Simplify the GC allocation logic using the implementation from bug 1005844.

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

Nice improvement.

::: js/src/gc/Memory.cpp
@@ +52,5 @@
> +// Data from OOM crashes shows there may be up to 24 chunksized but unusable
> +// chunks available in low memory situations. These chunks may all need to be
> +// used up before we gain access to remaining *alignable* chunksized regions,
> +// so we use a generous limit of 32 unusable chunks to ensure we reach them.
> +static const int MaxLastDitchAttempts = 32;

Yay, real data!

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

(I'll request checkin on this now. It will probably fix the Coverity issue; if not it should make things easier to adjust regardless)

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8544838 [details] [diff] [review]
Simplify the GC allocation logic using the implementation from bug 1005844.

The code it was complaining about is removed, so I'd say that, yes, this clears up the issue. We'll need to wait until the next run of Coverity (once this lands) to see if any other issues arrise.

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

Right, in particular it should no longer use the value of remainingAddr after the memory it points to is deallocated (using the OS allocator). I'm guessing that's what it was getting upset about.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35faf9a888ae

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35faf9a888ae