Closed Bug 1341090 Opened 3 years ago Closed 3 years ago

Update malloc bytes count when allocating executable memory

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch execmem.patch (obsolete) — Splinter Review
This came up during bug 1332691, where a few tests create a lot of WasmInstanceObjects, each allocating its code segment, exhausting the executable memory (on 32 bits platforms in particular, where the threshold is low).

Actually, even a simple loop can trigger an OOM:

for(;;) {
    let i = new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary('(module)')));
}

Attached simple patch updates the malloc counter for executable memory allocations, causing GCs of WasmInstanceObjects, which then release the code segment and executable memory. At the end, the infinite loop really is infinite, and the tests in bug 1332691 pass on 32 bits.

I've decided to put the zone->updateMallocBytes() call in js::jit::AllocateExecutableMemory, because the alternative, calling js::jit::AllocateExecutableMemory *and* remember to call zone->updateMallocBytes() sounded error prone (for future invocations of AllocateExecutableMemory).
Attachment #8839218 - Flags: review?(jdemooij)
Attachment #8839218 - Flags: review?(jcoppeard)
Comment on attachment 8839218 [details] [diff] [review]
execmem.patch

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

LGTM.
Attachment #8839218 - Flags: review?(jcoppeard) → review+
Try build, just in case searchfox didn't catch all the calls to AllocateExecutableMemory: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7a372caff52536110c31bc55dd2c206951f62c1

Could this affect performance, since now we can GC when we allocate JIT code, thus discarding previous JIT code?
Comment on attachment 8839218 [details] [diff] [review]
execmem.patch

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

::: js/src/jit/ProcessExecutableMemory.cpp
@@ +611,2 @@
>  {
> +    cx->zone()->updateMallocCounter(bytes);

ExecutableAllocator is a pool allocator that's shared by all Zones in the runtime. Adding the whole pool size to the current Zone's malloc count seems wrong.

I think it would be more correct to have ExecutableAllocator call updateMallocCounter with the size of the current allocation (and a second updateMallocCounter call in AllocateCodeSegment).

(At some point it would be nice to move ExecutableAllocator to JitZone so we can do this, but that's a bigger change.)

::: js/src/jit/ProcessExecutableMemory.h
@@ +30,5 @@
>  extern MOZ_MUST_USE bool InitProcessExecutableMemory();
>  extern void ReleaseProcessExecutableMemory();
>  
>  // Allocate/deallocate executable pages.
> +extern void* AllocateExecutableMemory(JSContext* cx, size_t bytes, ProtectionSetting protection);

Nit: the Try push has non-unified build failures. Probably best to forward declare JSContext at the top of this file and include gc/Zone.h in the cpp file.
Attachment #8839218 - Flags: review?(jdemooij)
Attached patch alloc.patch (obsolete) — Splinter Review
Thanks for the review. Here's an updated patch, which even passes non-unified build locally.

Future callers of AllocateExecutableMemory will have to be careful to update the zone counter too. Fortunately, there might not be any new callers any time soon.
Attachment #8839218 - Attachment is obsolete: true
Attachment #8839410 - Flags: review?(jdemooij)
Comment on attachment 8839410 [details] [diff] [review]
alloc.patch

Discarding review: we've discussed the topic on irc and realized a new counter would actually be better, to prevent JIT code from being compiled and discarded just after the fact.
Attachment #8839410 - Flags: review?(jdemooij)
Attached patch 1.refactor.patch (obsolete) — Splinter Review
This refactors a bit the way memory counters are implemented, since there was some redundancy between the GC level and the Zone level. Also, the jit code counter is trivial after this.
Attachment #8839410 - Attachment is obsolete: true
Attachment #8839606 - Flags: review?(jcoppeard)
This implements the jit counter, and uses a very high fraction of the max allocatable executable memory as a threshold. It seems pretty safe even on 32 bits, and the iloop in comment 0 never OOMs with it.
Attachment #8839608 - Flags: review?(jdemooij)
Attachment #8839608 - Flags: review?(jcoppeard)
Attached patch 1.refactor.patch (obsolete) — Splinter Review
(forgot to fold patches)
Attachment #8839606 - Attachment is obsolete: true
Attachment #8839606 - Flags: review?(jcoppeard)
Attachment #8839609 - Flags: review?(jcoppeard)
Comment on attachment 8839608 [details] [diff] [review]
2.jitcodecounter.patch

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

Looks good. I agree we should be okay with that 0.9 factor.
Attachment #8839608 - Flags: review?(jdemooij) → review+
Comment on attachment 8839609 [details] [diff] [review]
1.refactor.patch

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

Thanks for the refactoring.  This is much better than having (almost) the same code in three places.

::: js/src/gc/GCRuntime.h
@@ +644,5 @@
> +            triggered_ = triggerGC();
> +    }
> +
> +    virtual bool triggerGC() = 0;
> +

Rather than using a virtual method, it might be easier to make this a template class (on either Zone* or GCRuntime*) and have update() take a pointer on which it calls onTooMuchMalloc().  You'd want to check the triggered flag first so that check remains factored out.

@@ +781,5 @@
> +    int32_t getMallocBytes() const { return mallocCounter.bytes(); }
> +    size_t maxMallocBytesAllocated() const { return mallocCounter.maxBytes(); }
> +    bool isTooMuchMalloc() const { return mallocCounter.isTooMuchMalloc(); }
> +    void onTooMuchMalloc() { mallocCounter.onTooMuchMalloc(); }
> +    void resetMallocBytes() { mallocCounter.reset(); }

You're adding methods for pretty much all the public methods of MemoryCounter.  It might be simpler just to make mallocCounter public.
Attachment #8839608 - Flags: review?(jcoppeard) → review+
Thanks for the reviews! Now that bug 1337561 has a specific fix for wasm, I wonder if patch 2 is still valuable: it might help for Ion code, in case a single zone generates an awful lot of code.

Alternatively, we could also reuse the largeAllocFailureCallback in the Linker's code, to retry allocating if it failed; that might trash previous JIT code at the expense of a bigger JIT code chunk, but it somehow makes sense that this happens (more inlining probably implies more code, and the trashed code might have got inlined).

Jon, Jan, what do you think?
(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> Alternatively, we could also reuse the largeAllocFailureCallback in the
> Linker's code, to retry allocating if it failed; that might trash previous
> JIT code at the expense of a bigger JIT code chunk, but it somehow makes
> sense that this happens (more inlining probably implies more code, and the
> trashed code might have got inlined).

We have to be careful: I think Ion at least allocates code in a no-gc scope, and we shouldn't discard the script's Baseline code while finishing its Ion code (to not break the invariant that all scripts with Ion code have Baseline code - we cancel off-thread compilations when discarding code to ensure this).

Also, we stop JIT compiling when CanLikelyAllocateMoreExecutableMemory returns false. I'm not sure we need to do anything else here, most JIT allocations are small so fragmentation is less of an issue and we discard code quite aggressively.
As discussed on irc, keeping the methods encapsulating the Zone counter, since updateMallocBytes is needed for MallocProvider, and reset + isTooMuchMalloc get merged in the following patches, leaving only two proxy methods.
Attachment #8839609 - Attachment is obsolete: true
Attachment #8839609 - Flags: review?(jcoppeard)
Attachment #8840385 - Flags: review?(jcoppeard)
Comment on attachment 8840385 [details] [diff] [review]
1.refactoring.patch

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

Thanks for the updates, this looks good.

::: js/src/gc/GCRuntime.h
@@ +635,5 @@
> +    void update(T* owner, size_t bytes, bool* triggeredGC = nullptr) {
> +        bytes_ -= ptrdiff_t(bytes);
> +        if (MOZ_UNLIKELY(isTooMuchMalloc())) {
> +            if (!triggered_)
> +                triggered_ = owner->triggerGC();

Please call this method something like triggerGCForTooMuchMalloc().

@@ +638,5 @@
> +            if (!triggered_)
> +                triggered_ = owner->triggerGC();
> +        }
> +        if (triggeredGC)
> +            *triggeredGC = triggered_;

Oh, I meant to comment before: can you just return triggered rather than using an outparam?
Attachment #8840385 - Flags: review?(jcoppeard) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95361085cae4
Refactor malloc counters in Zone and GCRuntime; r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e4839619c8
Implement a JIT code counter; r=jonco, r=jandem
For the record: it was discussed on irc with jandem to use 0.8 as the threshold fraction instead of 0.9.
https://hg.mozilla.org/mozilla-central/rev/95361085cae4
https://hg.mozilla.org/mozilla-central/rev/23e4839619c8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.