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

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 235192: Update ARM64EHash
https://bugs.webkit.org/show_bug.cgi?id=235192

Attachment 449236: patch

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




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

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)

Make ALWAYS_INLINE.

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

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

/Addition/Allocation/

> 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)

ALWAYS_INLINE.

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

ALWAYS_INLINE.

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

ALWAYS_INLINE.

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

ALWAYS_INLINE.

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

ALWAYS_INLINE?

> 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.