There are some ARM64 devices that have runtime support for APRR, but the current way we link in copyCompactAndLinkCode() causes us to use a side buffer to link the code before copying the results to the JIT memory. By making the FAST_JIT_PERMISSION paths runtime selected, we can eliminate the allocation of the buffer and the subsequent copying out of the buffer. This is a follow to https://bugs.webkit.org/show_bug.cgi?id=212765.
<rdar://problem/64030110>
Created attachment 401158 [details] Patch
Created attachment 401161 [details] Patch with speculative build fixes
Created attachment 401176 [details] Patch More changes to make the code statically compiled where appropriate. This should fix the ARMv7 build.
Comment on attachment 401176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401176&action=review > Source/JavaScriptCore/assembler/LinkBuffer.cpp:282 > + IF_FAST_JIT_PERMISSIONS(os_thread_self_restrict_rwx_to_rw()); why not just use a normal if statement here? Doesn't feel like canonical webkit style. I think if (useFastJITPermissions()) is much easier to parse and read than macros as control flow. > Source/JavaScriptCore/assembler/LinkBuffer.cpp:371 > + IF_FAST_JIT_PERMISSIONS_ELSE(MacroAssembler::link<memcpy>(jumpsToLink[i], outData + jumpsToLink[i].from(), location, target), \ > + MacroAssembler::link<performJITMemcpy>(jumpsToLink[i], outData + jumpsToLink[i].from(), location, target)); same comment
Created attachment 401188 [details] Patch with suggested changes (In reply to Saam Barati from comment #5) > Comment on attachment 401176 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401176&action=review > > > Source/JavaScriptCore/assembler/LinkBuffer.cpp:282 > > + IF_FAST_JIT_PERMISSIONS(os_thread_self_restrict_rwx_to_rw()); > > why not just use a normal if statement here? Doesn't feel like canonical > webkit style. > > I think > if (useFastJITPermissions()) > is much easier to parse and read than macros as control flow. > > > Source/JavaScriptCore/assembler/LinkBuffer.cpp:371 > > + IF_FAST_JIT_PERMISSIONS_ELSE(MacroAssembler::link<memcpy>(jumpsToLink[i], outData + jumpsToLink[i].from(), location, target), \ > > + MacroAssembler::link<performJITMemcpy>(jumpsToLink[i], outData + jumpsToLink[i].from(), location, target)); > > same comment Here's a patch with the suggested changes.
Comment on attachment 401188 [details] Patch with suggested changes View in context: https://bugs.webkit.org/attachment.cgi?id=401188&action=review > Source/JavaScriptCore/assembler/LinkBuffer.cpp:137 > +ALWAYS_INLINE bool useFastJITPermissions() > +{ > +#if ENABLE(FAST_JIT_PERMISSIONS) && !ENABLE(SEPARATED_WX_HEAP) > + return true; > +#elif ENABLE(FAST_JIT_PERMISSIONS) > + return g_jscConfig.useFastPermisionsJITCopy; > +#else > + return false; > +#endif > +} If you moved this to ExecutableAllocator.h, you could use useFastJITPermissions() to slightly simplify performJITMemcpy as well (remove one #if/endif ENABLE(SEPARATED_WX_HEAP)). > Source/JavaScriptCore/assembler/LinkBuffer.cpp:268 > + os_thread_self_restrict_rwx_to_rw(); Is this still in an #ifdef that is Darwin specific? Or are we just getting away with this because we only build branch compaction on Darwin systems?
Comment on attachment 401188 [details] Patch with suggested changes View in context: https://bugs.webkit.org/attachment.cgi?id=401188&action=review r=me > Source/JavaScriptCore/ChangeLog:8 > + Made the FAST_JIT_PERMISSIONS code to be runtime conditional. "code to be" => maybe something a bit more specific: "code when linking to be" > Source/JavaScriptCore/ChangeLog:16 > + Broke out the "verify hash" conditionally compiled code with a file local > + ENABLE_VERIFY_JIT_HASH macro for readability. this is nicer > Source/JavaScriptCore/assembler/LinkBuffer.cpp:250 > +#if ENABLE(VERIFY_JIT_HASH) can we assert that useFastJITPermissions() is true alternatively, just return true from that function #if ENABLE(VERIFY_JIT_HASH)
Comment on attachment 401188 [details] Patch with suggested changes View in context: https://bugs.webkit.org/attachment.cgi?id=401188&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + Made the FAST_JIT_PERMISSIONS code to be runtime conditional. > > "code to be" => maybe something a bit more specific: "code when linking to be" or maybe even more specific, say something about the side buffer vs malloc buffer.
(In reply to Sam Weinig from comment #7) > Comment on attachment 401188 [details] > Patch with suggested changes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401188&action=review > > > Source/JavaScriptCore/assembler/LinkBuffer.cpp:137 > > +ALWAYS_INLINE bool useFastJITPermissions() > > +{ > > +#if ENABLE(FAST_JIT_PERMISSIONS) && !ENABLE(SEPARATED_WX_HEAP) > > + return true; > > +#elif ENABLE(FAST_JIT_PERMISSIONS) > > + return g_jscConfig.useFastPermisionsJITCopy; > > +#else > > + return false; > > +#endif > > +} > > If you moved this to ExecutableAllocator.h, you could use > useFastJITPermissions() to slightly simplify performJITMemcpy as well > (remove one #if/endif ENABLE(SEPARATED_WX_HEAP)). Let me look into that. > > Source/JavaScriptCore/assembler/LinkBuffer.cpp:268 > > + os_thread_self_restrict_rwx_to_rw(); > > Is this still in an #ifdef that is Darwin specific? Or are we just getting > away with this because we only build branch compaction on Darwin systems? Yes it is. I posted this patch to see if ALWAYS_LINE could get the other compilers / SDKs to work, but it wasn't successful.
r=me still with old Macro control flow version, after talking with Michael, compiling in the control flow is leading to build annoyance
Created attachment 401220 [details] Patch for Landing > > Source/JavaScriptCore/assembler/LinkBuffer.cpp:137 > > +ALWAYS_INLINE bool useFastJITPermissions() > > +{ > > +#if ENABLE(FAST_JIT_PERMISSIONS) && !ENABLE(SEPARATED_WX_HEAP) > > + return true; > > +#elif ENABLE(FAST_JIT_PERMISSIONS) > > + return g_jscConfig.useFastPermisionsJITCopy; > > +#else > > + return false; > > +#endif > > +} > > If you moved this to ExecutableAllocator.h, you could use > useFastJITPermissions() to slightly simplify performJITMemcpy as well > (remove one #if/endif ENABLE(SEPARATED_WX_HEAP)). Done. > > Source/JavaScriptCore/assembler/LinkBuffer.cpp:250 > > +#if ENABLE(VERIFY_JIT_HASH) > > can we assert that useFastJITPermissions() is true > > alternatively, just return true from that function #if > ENABLE(VERIFY_JIT_HASH) Although these sections of code were part of the same "#if CPU(ARM64E) && ENABLE(FAST_JIT_PERMISSIONS)" sections, this is actually independent of FAST_JIT_PERMISSIONS. You'll see elsewhere (AssemblerBuffer.h ~183) that the assembly hash code is simply #if CPU(ARM64E). I have adjusted the setting of ENABLE_VERIFY_JIT_HASH accordingly.
Committed r262670: <https://trac.webkit.org/changeset/262670>