Bug 240407 – Shrink Structure to 96 Bytes when addresses can be encoded in 36-bits.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yijia Huang]

...

[Yijia Huang]

Created attachment 459379 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 459379 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        REGRESSION (r89377554): [iOS] %2 RAMification

This is not a WebKit revision.

[Yijia Huang]

(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 459379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459379&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        REGRESSION (r89377554): [iOS] %2 RAMification
> 
> This is not a WebKit revision.

Sorry, I though "r" stands for radar.

[Justin Michaud]

Comment on attachment 459379 [details]
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.

[Justin Michaud]

(In reply to Yijia Huang from comment #3)
> (In reply to Alexey Proskuryakov from comment #2)
> > Comment on attachment 459379 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=459379&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:3
> > > +        REGRESSION (r89377554): [iOS] %2 RAMification
> > 
> > This is not a WebKit revision.
> 
> Sorry, I though "r" stands for radar.

For radars, we use rdar://000000

[Justin Michaud]

cc'ing the radar bot and adding the InRadar tag: <rdar://89377554>

[Yijia Huang]

Comment on attachment 459379 [details]
Patch

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

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

m_jit.branchCompactPtr requires additional scratchGPR. How can we handle:

/Volumes/WebKit/ios/OpenSource/Source/JavaScriptCore/jit/AssemblyHelpers.h:1928:88: error: unused parameter 'scratch' [-Werror,-Wunused-parameter]
    Jump branchCompactPtr(RelationalCondition cond, GPRReg left, Address right, GPRReg scratch)

, when it is not iOS platform?

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

The Bits is actually being used somewhere else. Should we use `uintptr_t` for `Compacted::StorageSize` for the non-iOS platforms instead of T*? Then, we can directly use `using StorageSize = WTF::Compacted<nullptr_t>::StorageSize;`

[Yijia Huang]

Created attachment 459467 [details]
Patch

[Yijia Huang]

Created attachment 459469 [details]
Patch

[Yijia Huang]

Created attachment 459470 [details]
Patch

[Mark Lam]

Comment on attachment 459470 [details]
Patch

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

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1936
> +        (void)scratch;

Use UNUSED_PARAM(scratch).

[Yijia Huang]

Pull request: https://github.com/WebKit/WebKit/pull/658

[Mark Lam]

Comment on attachment 459470 [details]
Patch

This patch is obsolete.  The current patch is at the PR on GitHub.

[Yijia Huang]

Pull request: https://github.com/WebKit/WebKit/pull/889

[Yijia Huang]

Pull request: https://github.com/WebKit/WebKit/pull/894

[Yijia Huang]

Pull request: https://github.com/WebKit/WebKit/pull/896

[Yijia Huang]

Pull request: https://github.com/WebKit/WebKit/pull/963

[Yijia Huang]

Pull request: https://github.com/WebKit/WebKit/pull/966

[Yijia Huang]

Created attachment 459705 [details]
Patch

[Yijia Huang]

Pull request: https://github.com/WebKit/WebKit/pull/983

[EWS]

Committed r295008 (251103@main): <https://commits.webkit.org/251103@main>

Reviewed commits have been landed. Closing PR #658 and removing active labels.

[Yijia Huang]

Landed Commit: https://github.com/WebKit/WebKit/commit/771382d8a3be082672a5f071b45375d6d3d36219
Fix for iOS debug build: https://github.com/WebKit/WebKit/commit/b9d14b3aaf16c9647fe0bb6abac487c5585ce737