[webkit-reviews] review denied: [Bug 191793] JSC should have a SPI to disable JIT after initializeThreading but before execution. : [Attachment 355162] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 191793: JSC should have a SPI to disable JIT after initializeThreading but
before execution.
https://bugs.webkit.org/show_bug.cgi?id=191793

Attachment 355162: Patch

https://bugs.webkit.org/attachment.cgi?id=355162&action=review




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 355162
  --> https://bugs.webkit.org/attachment.cgi?id=355162
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355162&action=review

What worries me about this patch is VM::canUseJIT/assembler. We compute those
things once. It's really weird to have the executable allocator go from
isValid() -> !isValid(). I'm worried that this may put us into a weird state
we're not accounting for. I think at the very least, this patch needs some
mechanism of transitioning the vm->canUseJIT()/assumbler/etc to false as well.
Or asserting that they haven't been computed yet either. IIRC, I added a call
to canUseJIT() in initialize threading for mini mode.

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:197
> +   
RELEASE_ASSERT(!PageReservation::reserveWithGuardPages(roundUpToMultipleOf(page
Size(), fixedExecutableMemoryPoolSize), OSAllocator::JSJITCodePages,
EXECUTABLE_POOL_WRITABLE, true));

I vote for RELEASE_ASSERT_WITH_MESSAGE here saying that a second JIT allocation
should fail on iOS

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:430
> +    allocator->preventAllocations();

let's also call this preventAllocationsOrCrash

> Source/WTF/wtf/MetaAllocator.h:103
> +    bool hasAllocated() { return m_hasAllocated; }

style nit: should be a const function