[webkit-reviews] review denied: [Bug 240407] REGRESSION: [iOS] %2 RAMification : [Attachment 459379] Patch

[bugzilla-daemon at webkit.org]

Justin Michaud <justin_michaud at apple.com> has denied Yijia Huang
<yijia_huang at apple.com>'s request for review:
Bug 240407: REGRESSION: [iOS] %2 RAMification
https://bugs.webkit.org/show_bug.cgi?id=240407

Attachment 459379: Patch

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




--- Comment #4 from Justin Michaud <justin_michaud at apple.com> ---
Comment on attachment 459379
  --> https://bugs.webkit.org/attachment.cgi?id=459379
Patch

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

Looking good, I left a few comments that might help you with your issues.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:14866
> +    slowCases.append(m_jit.branchCompactPtr(CCallHelpers::NotEqual,
scratch1GPR, CCallHelpers::Address(structureGPR, Structure::classInfoOffset()),
scratch2GPR));

Can we make branchCompactPtr do the right thing so we don't need these #ifs
everywhere?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:14872
>  

> emitAllocateJSObjectWithKnownSize<JSInternalPromise>(resultGPR, structureGPR,
butterfly, scratch1GPR, scratch2GPR, slowCases, sizeof(JSInternalPromise));

looks like scratch2 is still live?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:14924
>      slowCases.append(m_jit.branchPtr(CCallHelpers::NotEqual, scratch1GPR,
CCallHelpers::Address(structureGPR, Structure::globalObjectOffset())));

ditto for both

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:16838
> +	   return m_out.lShr(structure,
m_out.constInt32(StructureID::encodeShiftAmount));

need a b3 cast? This would only cause a validation error when JSC is running
the B3 compiler, not a runtime error

> Source/JavaScriptCore/heap/TinyBloomFilter.h:36
> +    using StorageSize = WTF::Compacted<nullptr_t>::StorageSize;

We can get rid of Bits, and get rid of the #if here. StorageSize should pick
the right size, right? Maybe let's name it StorageType or StorageBits or Bits?

> Source/JavaScriptCore/jit/AssemblyHelpers.h:-1625
> -	   load64(MacroAssembler::Address(structure,
Structure::structureIDOffset()), scratch);

Yusuke should leave a comment here. This seems correct, but I am not sure if we
can leave a JSCell in this state. We might need to do this atomically to avoid
GC tearing. This might happen if the GC would visit this cell in between these
two stores.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1930
> +    {

Let's define this for all platforms and have it do the right thing

> Source/JavaScriptCore/runtime/Structure.h:955
> +    static constexpr int s_maxTransitionLength = 64; // macOS|iOS: 4 bytes

Not sue about the comment here? Also, constexpr values don't necessarily have
to take up any space. Static values also don't contribute to object size.

> Source/WTF/wtf/CompactPtr.h:28
> +#include <cstdint>

Not sure we need all of these

> Source/WTF/wtf/CompactRefPtr.h:38
> +    WTF_MAKE_FAST_ALLOCATED;

I don't think we want this at all. We want this to be used only as a member of
a class. Also, maybe we should see if we can assert that it isn't stack
allocated, since that will break leaks.