[webkit-reviews] review granted: [Bug 235192] Update ARM64EHash : [Attachment 449236] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 18 15:01:55 PST 2022

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 235192: Update ARM64EHash

Attachment 449236: patch


--- Comment #13 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 449236
  --> https://bugs.webkit.org/attachment.cgi?id=449236

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

r=me with comments.

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:211
> +	   ARM64EHash()

nit: you can just express this is as `ARM64EHash() = default;`

> Source/JavaScriptCore/assembler/SecureARM64EHashPins.cpp:76
> +    return memory;

Let's return the tagged memory instead, and untag it just before use.

> Source/JavaScriptCore/assembler/SecureARM64EHashPins.cpp:79
> +SecureARM64EHashPins::Page::Page()

Let's make this ALWAYS_INLINE.

> Source/JavaScriptCore/assembler/SecureARM64EHashPins.cpp:88
> +static void initializePage(const WriteToJITRegionScope&,
SecureARM64EHashPins::Page* page)


> Source/JavaScriptCore/assembler/SecureARM64EHashPins.cpp:116
> +bool SecureARM64EHashPins::allocatePinForCurrentThreadImpl(const

nit: maybe make the hashPinsLock a static field and make this method
WTF_GUARDED_BY_LOCK by the static lock?

> Source/JavaScriptCore/assembler/SecureARM64EHashPins.cpp:141
> +    auto findResult = findEntry();

Let's rename findEntry() to findFirstEntry().  It'll document better what we're
actually aiming to do here.

> Source/JavaScriptCore/assembler/SecureARM64EHashPins.cpp:195
> +	       initializePage(writeScope, nextPage);

Let's init nextPage by untagging the return value of
allocateInExecutableMemory() here just before we use it.

> Source/JavaScriptCore/assembler/SecureARM64EHashPins.cpp:234
> +    // like data structure. The in use pin (top of the stack) will always be
at the lowest
> +    // page and lowest index in the bit vector. So when we deallocate the
current top of the

"the lowest page and lowest index in the bit vector" reads like it's always in
entry 0.  How about adding a qualifier: "the lowest page and lowest index in
the bit vector for that thread key."?

> Source/JavaScriptCore/assembler/SecureARM64EHashPins.cpp:238
> +    // Addition also maintains this invariant by always placing the newest
entry for


> Source/JavaScriptCore/assembler/SecureARM64EHashPins.h:39
> +    JS_EXPORT_PRIVATE uint64_t pinForCurrentThread();

This `JS_EXPORT_PRIVATE` is deceptive since this is an ALWAYS_INLINE function.

> Source/JavaScriptCore/assembler/SecureARM64EHashPins.h:63
> +	   inline Entry& fastEntryUnchecked(size_t index)


> Source/JavaScriptCore/assembler/SecureARM64EHashPins.h:69
> +	   void setIsInUse(size_t index)


> Source/JavaScriptCore/assembler/SecureARM64EHashPins.h:74
> +	   void clearIsInUse(size_t index)


> Source/JavaScriptCore/assembler/SecureARM64EHashPins.h:79
> +	   bool isInUse(size_t index)


> Source/JavaScriptCore/assembler/SecureARM64EHashPins.h:90
> +	   void forEachSetBit(Function function)


> Source/JavaScriptCore/assembler/SecureARM64EHashPins.h:105
> +	   Atomic<uint64_t> nextPin { 1 };
> +	   Atomic<uint32_t> assertNotReentrant { 0 };

Can you static_assert that nextPin comes before assertNotReentrant?  I think
part of our algorithm relies on this.

More information about the webkit-reviews mailing list