Closed Bug 1400278 Opened 2 years ago Closed 2 years ago

Attribute malloc memory usage to the Zone rather than the Runtime

Categories

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

55 Branch
enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p2])

Attachments

(6 files, 3 obsolete files)

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).
Whiteboard: [qf:p2]
Move ZoneAllocPolicy definition out of Zone.h to prevent cyclic include dependencies later.
Attachment #8909726 - Flags: review?(sphink)
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.
Attachment #8909727 - Flags: review?(jdemooij)
Replace all uses of RuntimeAllocPolicy with ZoneAllocPolicy.
Attachment #8909729 - Flags: review?(sphink)
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.
Attachment #8909734 - Flags: review?(jdemooij)
Following these patches we can almost remove RuntimeAllocPolicy and the allocation methods on JSRuntime, except for one use by wasm.
Attachment #8909726 - Flags: review?(sphink) → review+
Attachment #8909729 - Flags: review?(sphink) → review+
Attachment #8909727 - Flags: review?(jdemooij) → review+
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?
Attachment #8909734 - Flags: review?(jdemooij) → review+
(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.
Keywords: leave-open
Pushed by jcoppeard@mozilla.com:
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
Priority: -- → P2
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.
(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
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.
Attachment #8911178 - Flags: review?(sphink)
Attachment #8911178 - Attachment is obsolete: true
Attachment #8911178 - Flags: review?(sphink)
Attached patch bug1400278-trace-malloc-stub (obsolete) — Splinter Review
Sorry about that, I uploaded the wrong patch.
Attachment #8911225 - Flags: review?(sphink)
Attachment #8911225 - Flags: review?(sphink) → review+
This is now causing jit test debug/bug1370905.js to time out.  Investigating.
Attachment #8911225 - Attachment is obsolete: true
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.
Attachment #8909734 - Attachment is obsolete: true
Attachment #8912270 - Flags: review?(jdemooij)
Remove RuntimeAllocPolicy now it's no longer used, but leave allocation methods on JSRuntime which are.
Attachment #8912796 - Flags: review?(sphink)
Attachment #8912796 - Flags: review?(sphink) → review+
Comment on attachment 8912270 [details] [diff] [review]
bug1400278-dont-use-runtime-alloc v2

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

Sorry for the delay!
Attachment #8912270 - Flags: review?(jdemooij) → review+
Pushed by jcoppeard@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.