Bug 208490 – Implement 1GB of executable memory on arm64

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 392317 [details]
WIP

It begins

[Saam Barati]

Created attachment 392359 [details]
WIP

[Saam Barati]

Created attachment 392370 [details]
WIP

[Saam Barati]

Created attachment 392529 [details]
WIP

[Saam Barati]

Created attachment 392608 [details]
WIP

[Saam Barati]

Created attachment 392663 [details]
WIP

[Saam Barati]

Created attachment 392822 [details]
WIP

Seems to work. Now, I need to implement freeing islands and pick the correct region sizes.

[Saam Barati]

Created attachment 392836 [details]
WIP

[Saam Barati]

Created attachment 392983 [details]
WIP

[Saam Barati]

Created attachment 392984 [details]
WIP

[Saam Barati]

Created attachment 392985 [details]
WIP

[Saam Barati]

Created attachment 393107 [details]
WIP

I think it might be complete. Time to clean up the code

[Saam Barati]

Created attachment 393118 [details]
WIP

I need to make jump islands work with live repatching wasm code. Currently, we might load the wrong thing from the icache :-(

[Saam Barati]

Created attachment 393184 [details]
WIP

[Saam Barati]

(In reply to Saam Barati from comment #14)
> Created attachment 393184 [details]
> WIP

I solved my wasm bug. Turns out, when hot patching code concurrently, you need to do a few things to not SIGILL (surprise surprise...). I need to clean up the code again after my wasm debugging session, and then it should be ready for reviews.

Running JS2, and using the random allocation algorithm, our max jump island utilization rate is about 1%. So if we use 16MB, we should have a lot of leeway.

[Saam Barati]

Created attachment 393403 [details]
patch

[Saam Barati]

Created attachment 393406 [details]
patch

[Saam Barati]

Created attachment 393412 [details]
patch

try to fix testwtf build error

[Saam Barati]

Created attachment 393413 [details]
patch

testwtf build fix take 2

[Saam Barati]

Created attachment 393415 [details]
patch

testwtf build fix take 3

[Saam Barati]

Created attachment 393440 [details]
patch

[Saam Barati]

Created attachment 393444 [details]
patch

[Saam Barati]

Created attachment 393449 [details]
patch

[Keith Miller]

Comment on attachment 393449 [details]
patch

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

r=me if you make the requested changes and fix Win build.

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:113
> +#if USE(JUMP_ISLANDS)
> +static constexpr size_t fixedExecutableMemoryPoolSize = 1 * 1024 * 1024 * 1024;
> +// These sizes guarantee that any jump within an island can jump forwards or backwards
> +// to the adjacent island in a single instruction.
> +static constexpr size_t regionSize = 112 * 1024 * 1024;
> +static constexpr size_t islandRegionSize = 16 * 1024 * 1024;
> +static constexpr size_t numberOfRegions = fixedExecutableMemoryPoolSize / regionSize;
> +static constexpr size_t islandSizeInBytes = 4;
> +static constexpr size_t maxIslandsPerRegion = islandRegionSize / islandSizeInBytes;
> +#else
>  static constexpr size_t fixedExecutableMemoryPoolSize = 128 * 1024 * 1024;
> +#endif
>  #elif CPU(X86_64)
> -static constexpr size_t fixedExecutableMemoryPoolSize = 1024 * 1024 * 1024;
> +static constexpr size_t fixedExecutableMemoryPoolSize = 1 * 1024 * 1024 * 1024;

Can we switch to using the MB/GB constants here?

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:815
> +        CodeLocationLabel<ExecutableMemoryPtrTag> rootJumpLocation;

Can we call this jumpSourceLocation? rootJumpLocation doesn't really make it clear what this is to me. I thought it was the destination for a while, which clearly would have been borked.

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:824
> +    RedBlackTree<Islands, void*> islandsForJumpLocation;

ditto on changing this to m_islandsForJumpSourceLocation. You also don't have an m_ prefix which is weird.

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:173
> +        for (unsigned i = 0; i < m_codeBlock->m_wasmToWasmCallsites.size(); ++i) {
> +            stageRepatch(m_codeBlock->m_wasmToWasmCallsites[i]);
> +            if (m_codeBlock->m_llintCallees) {
> +                LLIntCallee& llintCallee = m_codeBlock->m_llintCallees->at(i).get();
> +                if (JITCallee* replacementCallee = llintCallee.replacement())
> +                    stageRepatch(replacementCallee->wasmToWasmCallsites());
> +                if (OMGForOSREntryCallee* osrEntryCallee = llintCallee.osrEntryCallee())
> +                    stageRepatch(osrEntryCallee->wasmToWasmCallsites());
> +            }
> +            if (BBQCallee* bbqCallee = m_codeBlock->m_bbqCallees[i].get()) {
> +                if (OMGCallee* replacementCallee = bbqCallee->replacement())
> +                    stageRepatch(replacementCallee->wasmToWasmCallsites());
> +                if (OMGForOSREntryCallee* osrEntryCallee = bbqCallee->osrEntryCallee())
> +                    stageRepatch(osrEntryCallee->wasmToWasmCallsites());
> +            }
> +        }

Seems like we could have some code deduplication between the different plans... Can you do that?

> Tools/ChangeLog:8
> +        * Scripts/run-jsc-stress-tests:

I think you need to update this for the TestWTF changes? Can you also add a TestWTF for RedBlackTree::iterate?

[Radar WebKit Bug Importer]

<rdar://problem/60797127>

[Saam Barati]

I'm addressing Keith's comments.

[Saam Barati]

Created attachment 394534 [details]
patch for landing

[Saam Barati]

Created attachment 394639 [details]
patch for landing

[EWS]

Committed r259582: <https://trac.webkit.org/changeset/259582>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394639 [details].