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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 19 21:44:06 PDT 2019

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

Attachment 365251: Patch


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

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


> Source/JavaScriptCore/bytecode/CodeOrigin.h:43

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

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 {


> 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

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

style: remove this whitespace.

More information about the webkit-reviews mailing list