[webkit-reviews] review granted: [Bug 195928] Compress CodeOrigin into a single word in the common case : [Attachment 365251] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 195928: Compress CodeOrigin into a single word in the common case
https://bugs.webkit.org/show_bug.cgi?id=195928

Attachment 365251: Patch

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




--- Comment #20 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 365251
  --> https://bugs.webkit.org/attachment.cgi?id=365251
Patch

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

r=me

> Source/JavaScriptCore/bytecode/CodeOrigin.h:43
> +#define HAS_FREE_BITS_AT_TOP_OF_POINTERS 0

can we undef this at the end of the header?

> Source/JavaScriptCore/bytecode/CodeOrigin.h:108
> +    // For the same reason we must provide explicit copy and move
constructors.

nit: Not sure we really need this comment given above comment sets the stage.
Perhaps just drop assignment operators from above.

Or you could remove the comments entirely. It's pretty self evident from
reading the code what's going on.

> Source/JavaScriptCore/bytecode/CodeOrigin.h:111
> +	   // We don't use the initializer list because it would not let us
optimize the common case where there is no out-of-line storage

initializer list?

> Source/JavaScriptCore/bytecode/CodeOrigin.h:209
> +    struct OutOfLineCodeOrigin {

make this WTF_MAKE_FAST_ALLOCATED

> Source/JavaScriptCore/bytecode/CodeOrigin.h:233
> +	   auto value = static_cast<uintptr_t>(1<<3);

style: 1<<3 => 1 << 3

> Source/JavaScriptCore/bytecode/CodeOrigin.h:242
> +    static const unsigned s_freeBitsAtTop = 16;

nit: I'd make all of these constexpr.

> Source/JavaScriptCore/bytecode/CodeOrigin.h:247
> +    // On ARM64 cpus we only use the bottom 36 bits of pointers so we've got
an enormous 28 free bits at the top.
> +    // If we could guarantee the value of
Options::maximumOptimizationCandidateInstructionCount at compile-time, we could
completely avoid the path for OutOfLine allocation.
> +    // Since we cannot guarantee that this option will never get ludicrously
large, we just let this in-practice dead branch around. It should be trivial to
predict anyway.

Not sure this comment is really adding much since we use CodeOrigin outside of
the compiler as well. So we'd need to never generate a CodeBlock w/ >= 2^28
instructions.

> Source/JavaScriptCore/bytecode/CodeOrigin.h:269
> +    //      or a deletion marker for a hash table).

style: remove this whitespace.