Bug 229618

Summary: Make ARM64 and X86_64 probe code a little bit more efficient.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: dpino, ews-watchlist, jmason, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=229629
https://bugs.webkit.org/show_bug.cgi?id=229794
Attachments:
Description Flags
proposed patch.
ews-feeder: commit-queue-
proposed patch.
ews-feeder: commit-queue-
proposed patch. ysuzuki: review+

Description Mark Lam 2021-08-27 10:51:14 PDT
We were using an unnecessary indirect call when we can be using a direct call.
Comment 1 Radar WebKit Bug Importer 2021-08-27 10:51:43 PDT
<rdar://problem/82445743>
Comment 2 Mark Lam 2021-08-27 11:12:58 PDT
Created attachment 436642 [details]
proposed patch.
Comment 3 Mark Lam 2021-08-27 11:19:17 PDT
Created attachment 436643 [details]
proposed patch.
Comment 4 Yusuke Suzuki 2021-08-27 11:34:35 PDT
Comment on attachment 436643 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:18
> +        For MacroAssemblerX86Common.cpp, we left the X86 and MSVC implementations unchanged.
> +        For X86, I don't know the stack alignment requirements (if any) plus we might want
> +        to delete this code eventually since we're not supporting the X86 JIT anymore.

Right.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:-323
> -static_assert(JIT_PROBE_EXECUTOR_PTR_TAG == JITProbeExecutorPtrTag);

Let's remove JITProbeExecutorPtrTag from JSCPtrTag.h since it is no longer used.
Comment 5 Mark Lam 2021-08-27 11:36:47 PDT
(In reply to Yusuke Suzuki from comment #4)
> > Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:-323
> > -static_assert(JIT_PROBE_EXECUTOR_PTR_TAG == JITProbeExecutorPtrTag);
> 
> Let's remove JITProbeExecutorPtrTag from JSCPtrTag.h since it is no longer
> used.

Ah yes, I had planned to do that but forgot.
Comment 6 Mark Lam 2021-08-27 11:41:02 PDT
Created attachment 436647 [details]
proposed patch.
Comment 7 Yusuke Suzuki 2021-08-27 13:12:25 PDT
Comment on attachment 436647 [details]
proposed patch.

r=me
Comment 8 Mark Lam 2021-08-27 13:50:24 PDT
Thanks for the review.  Landed in r281718: <http://trac.webkit.org/r281718>.
Comment 9 Mark Lam 2021-08-27 14:19:31 PDT
This patch has a bug (accidentally deleted 2 lines) that is fixed in https://bugs.webkit.org/show_bug.cgi?id=229629.
Comment 10 Diego Pino 2021-09-02 00:58:36 PDT
This patch introduced a regression in Ubuntu 18.04. Compilation aborts on a linking error:

https://build.webkit.org/#/builders/71/builds/222

FAILED: lib/libjavascriptcoregtk-4.0.so.18.19.2
: && /usr/lib/ccache/g++-8 -fPIC -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wun
/usr/bin/ld.gold: error: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-cd2e8cfa-2.cpp.o: requires dynamic R_X86_64_PC32 reloc again
collect2: error: ld returned 1 exit status
[207/6083] Building CXX object Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/compiler/translator/tree_ops/RewriteAtomicFunctionExpressions.cpp.o
ninja: build stopped: subcommand failed.
program finished with exit code 254

This Ubuntu 18.04 bot uses GCC-8 (v8.4.0) and links with GNU Gold (GNU Binutils for Ubuntu 2.30 - 1.15).

On the Debian bot this regression didn't happen. It uses a similar compiler and linker. Compiler: v8.3.0. Linker: GNU Binutils for Debian 2.31.1 - 1.16.

Any clue or hint on what might have gone wrong?
There is a Debian bot with
Comment 11 Mark Lam 2021-09-02 01:16:58 PDT
(In reply to Diego Pino from comment #10)
> This patch introduced a regression in Ubuntu 18.04. Compilation aborts on a
> linking error:
> 
...
> /usr/bin/ld.gold: error:
> Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/
> DerivedSources/unified-sources/UnifiedSource-cd2e8cfa-2.cpp.o: requires
> dynamic R_X86_64_PC32 reloc again
> collect2: error: ld returned 1 exit status
...
> Any clue or hint on what might have gone wrong?
> There is a Debian bot with

Looks like your linker doesn't support linking the direct branch I used in the inline asm statement in ctiMasmProbeTrampoline.  I can try re-instating the old way of working for non-darwin ports since the diff isn't big in for X86_64.
Comment 12 Mark Lam 2021-09-02 01:27:49 PDT
(In reply to Mark Lam from comment #11)
> Looks like your linker doesn't support linking the direct branch I used in
> the inline asm statement in ctiMasmProbeTrampoline.  I can try re-instating
> the old way of working for non-darwin ports since the diff isn't big in for
> X86_64.

See https://bugs.webkit.org/show_bug.cgi?id=229794.
Comment 13 Jim Mason 2021-09-02 02:22:11 PDT
FYI, also getting an ld fatal error with the Sun linker.  It bails on the new X86_64 inline assembly instruction `call executeJSCJITProbe` (text relocation remains against symbol executeJSCJITProbe.)

If I change to `call executeJSCJITProbe@PLT`, it links and runs successfully.  I am patching it locally like this:

--- a/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp       2021-09-01 18:03:12.777668266 +0000
+++ b/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp       2021-09-01 19:00:37.913407269 +0000
@@ -596,7 +596,7 @@
     "movq %xmm15, " STRINGIZE_VALUE_OF(PROBE_CPU_XMM15_OFFSET) "(%rsp)" "\n"
 
     "movq %rsp, %rdi" "\n" // the Probe::State* arg.
-    "call " SYMBOL_STRING(executeJSCJITProbe) "\n"
+    "call " GLOBAL_REFERENCE(executeJSCJITProbe) "\n"
 
     // Make sure the Probe::State is entirely below the result stack pointer so
     // that register values are still preserved when we call the initializeStack

where the GLOBAL_REFERENCE macro is appending @PLT.

I may be the same linker issue Diego is reporting in Ubuntu.
Comment 14 Jim Mason 2021-09-02 04:04:29 PDT
I applied Yusuke's speculative patch (r281911) instead of my own, and can confirm it clears the issue with the Sun linker.