1400278 - Attribute malloc memory usage to the Zone rather than the Runtime

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P2)

Description

[Jon Coppeard (:jonco)]

Malloc memory counters are used to trigger GCs.  We have counters for the runtime and per zone which can trigger full and zone GCs respectively.  We'd like to move away from triggering full GCs and also improve our GC scheduling, so we should ensure we always attribute memory to its associated zone where possible.

In fact, all GC-reclaimable memory is associated with a zone, so we should never have to associate malloc memory with just a runtime.

(Note that when we increment the counter for a zone we also increment the runtime counter by that much as well.  When we do *any* GC we reset the runtime counter.  This is a little strange and I'd like to improve this too but that's the subject of another bug).

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1400278-zone-alloc-policy

Move ZoneAllocPolicy definition out of Zone.h to prevent cyclic include dependencies later.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1400278-fix-return-errors

For some reason the previous patch results in a bunch of compilation errors concerning unreachable return statements when built with FILES_PER_UNIFIED_FILE = 1.  This patch removes the unreachable statements.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1400278-use-zone-alloc-policy

Replace all uses of RuntimeAllocPolicy with ZoneAllocPolicy.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1400278-dont-use-runtime-alloc

Replace use of runtime-based allocation methods with either zone-based allocation for memory that is reclaimed on GC or with system allocation functions for things that are not reclaimed on GC.

Comment 5

[Jon Coppeard (:jonco)]

Following these patches we can almost remove RuntimeAllocPolicy and the allocation methods on JSRuntime, except for one use by wasm.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8909734 [details] [diff] [review]
bug1400278-dont-use-runtime-alloc

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

Makes sense.

::: js/src/jit/CompileWrappers.h
@@ +67,5 @@
>  
>      CompileRuntime* runtime();
>      bool isAtomsZone();
>  
> +    JitZone* jitZone();

I think it would be safer to have a method that returns just the malloc stub. That way we don't have to expose the full JitZone* (makes it easier to introduce races).

::: js/src/jit/VMFunctions.cpp
@@ -546,3 @@
>  {
>      AutoUnsafeCallWithABI unsafe;
> -    return rt->pod_malloc<uint8_t>(nbytes);

So we can get rid of rt->pod_malloc et al after this?

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #6)
> I think it would be safer to have a method that returns just the malloc
> stub. That way we don't have to expose the full JitZone* (makes it easier to
> introduce races).

Good idea.

> So we can get rid of rt->pod_malloc et al after this?

Wasm can allocate data that is shared between multiple runtimes and I haven't work out how best to account for that.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b95045e6223b
Move definition of ZoneAllocPolicy to jsalloc.h r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c960c257d13
Fix compilation errors to do with unreachable return statements r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/065aebfbdd75
Replace use of RuntimeAllocPolicy with ZoneAllocPolicy r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/29b2e8acf5f9
Replace runtime allocation with system or zone allocation where possible r=jandem

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e2869a877a6
Fix non-unified build bustage r=me

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa3fff074f32
Now fix style check bustage r=me

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b95045e6223b
https://hg.mozilla.org/mozilla-central/rev/6c960c257d13
https://hg.mozilla.org/mozilla-central/rev/065aebfbdd75
https://hg.mozilla.org/mozilla-central/rev/29b2e8acf5f9
https://hg.mozilla.org/mozilla-central/rev/5e2869a877a6
https://hg.mozilla.org/mozilla-central/rev/aa3fff074f32

Comment 12

[Jon Coppeard (:jonco)]

I'm seeing local crashes when running the octane typescript benchmark that appear to be related to the bug1400278-dont-use-runtime-alloc patch, so I'm backing this out.

Comment 13

[Jon Coppeard (:jonco)]

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ecf1fb0316d98aa760062060036cdbd36e38233b

Comment 14

[Jon Coppeard (:jonco)]

Attachment: backout-bug1400278-dont-use-runtime-alloc

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jon Coppeard (:jonco) from comment #13)
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> ecf1fb0316d98aa760062060036cdbd36e38233b

Pushed to Beta57 too.
https://hg.mozilla.org/releases/mozilla-beta/rev/e226c0b7420d

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged backout https://hg.mozilla.org/mozilla-central/rev/ecf1fb0316d9

Comment 17

[Jon Coppeard (:jonco)]

Attachment: backout-bug1400278-dont-use-runtime-alloc

I made the malloc stub be per-zone, but didn't trace it.  It turns out that runtime-wide stub jitcode is allocated in the atoms zone and we trace all jitcode cells in this zone in JitRuntime::Trace.

Comment 18

[Jon Coppeard (:jonco)]

Attachment: bug1400278-trace-malloc-stub

Sorry about that, I uploaded the wrong patch.

Comment 19

[Jon Coppeard (:jonco)]

This is now causing jit test debug/bug1370905.js to time out.  Investigating.

Comment 20

[Jon Coppeard (:jonco)]

Attachment: bug1400278-dont-use-runtime-alloc v2

In the previous version of this patch I forgot to trace the new per-zone malloc stub, which caused problems.  But we can't trace this unconditionally as that would always keep the zone alive.  It didn't seem simple to make this work so I changed the patch to go back to a single runtime-wide malloc stub and to pass in the zone as a parameter.  The malloc stub is used very rarely as far as I can tell so this shouldn't make any difference to performance.

Comment 21

[Jon Coppeard (:jonco)]

Attachment: bug1400278-remove-runtime-alloc-policy

Remove RuntimeAllocPolicy now it's no longer used, but leave allocation methods on JSRuntime which are.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8912270 [details] [diff] [review]
bug1400278-dont-use-runtime-alloc v2

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

Sorry for the delay!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6fce9f85d91
Replace runtime allocation with system or zone allocation where possible r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d19050cc86a
Remove RuntimeAllocPolicy now it's no longer used r=sfink

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f6fce9f85d91
https://hg.mozilla.org/mozilla-central/rev/3d19050cc86a