Bug 212765 - Add a Thread Specific Cache for LinkBuffer::CompactAndLinkCode()
Summary: Add a Thread Specific Cache for LinkBuffer::CompactAndLinkCode()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks: 212825
  Show dependency treegraph
 
Reported: 2020-06-04 11:15 PDT by Michael Saboff
Modified: 2020-06-05 09:27 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.28 KB, patch)
2020-06-04 11:23 PDT, Michael Saboff
saam: review+
Details | Formatted Diff | Diff
Patch for landing (4.21 KB, patch)
2020-06-04 13:09 PDT, Michael Saboff
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2020-06-04 11:15:32 PDT
Similar to https://bugs.webkit.org/show_bug.cgi?id=212562, we should add a Thread Local Buffer for LinkBuffer::copyCompactAndLinkCode().  When compacting JIT code, for most CPU types we need a second data buffer.  We should make this buffer a thread local just like we did for the AssemblerBuffer in 212562.
Comment 1 Michael Saboff 2020-06-04 11:17:32 PDT
<rdar://problem/63986742>
Comment 2 Michael Saboff 2020-06-04 11:23:00 PDT
Created attachment 401051 [details]
Patch
Comment 3 Saam Barati 2020-06-04 12:15:51 PDT
Comment on attachment 401051 [details]
Patch

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

r=me

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:125
> +typedef ThreadSpecific<BranchCompactionLinkBuffer, WTF::CanBeGCThread::True> ThreadSpecificBranchCompactionLinkBuffer;

nit: "using" instead of "typedef"

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:201
> +    uint8_t* m_data;
> +    size_t m_size;

nit: you can give these fields default values so you don't have to initialize them in 2 different ctors

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:232
> +    BranchCompactionLinkBuffer outBuffer(m_size);

can we do the FAST_JIT_PERMISSIONS&&arm64e path above in a runtime supported way for other devices too? Seems purely better than this, since it doesn't require a side buffer.

There are a few arm64 devices that at runtime support APRR

Maybe as a follow-up in a separate patch
Comment 4 Michael Saboff 2020-06-04 13:09:07 PDT
Created attachment 401067 [details]
Patch for landing

> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:125
> > +typedef ThreadSpecific<BranchCompactionLinkBuffer, WTF::CanBeGCThread::True> ThreadSpecificBranchCompactionLinkBuffer;
> 
> nit: "using" instead of "typedef"

Done.

> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:201
> > +    uint8_t* m_data;
> > +    size_t m_size;
> 
> nit: you can give these fields default values so you don't have to
> initialize them in 2 different ctors

Done.
Comment 5 Michael Saboff 2020-06-04 19:41:01 PDT
Committed r262591: <https://trac.webkit.org/changeset/262591>