Bug 240407 - Shrink Structure to 96 Bytes when addresses can be encoded in 36-bits.
Summary: Shrink Structure to 96 Bytes when addresses can be encoded in 36-bits.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Yijia Huang
URL:
Keywords: InRadar
Depends on:
Blocks: 233231
  Show dependency treegraph
 
Reported: 2022-05-13 17:14 PDT by Yijia Huang
Modified: 2022-05-31 10:50 PDT (History)
16 users (show)

See Also:


Attachments
Patch (39.61 KB, patch)
2022-05-15 09:45 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (75.70 KB, patch)
2022-05-16 17:42 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (75.70 KB, patch)
2022-05-16 17:51 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (75.30 KB, patch)
2022-05-16 18:04 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (91.96 KB, patch)
2022-05-24 00:22 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yijia Huang 2022-05-13 17:14:46 PDT
...
Comment 1 Yijia Huang 2022-05-15 09:45:08 PDT
Created attachment 459379 [details]
Patch
Comment 2 Alexey Proskuryakov 2022-05-16 09:13:17 PDT
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.
Comment 3 Yijia Huang 2022-05-16 09:15:33 PDT
(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.
Comment 4 Justin Michaud 2022-05-16 09:17:53 PDT
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.
Comment 5 Justin Michaud 2022-05-16 09:18:42 PDT
(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
Comment 6 Justin Michaud 2022-05-16 09:20:24 PDT
cc'ing the radar bot and adding the InRadar tag: <rdar://89377554>
Comment 7 Yijia Huang 2022-05-16 12:42:40 PDT
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;`
Comment 8 Yijia Huang 2022-05-16 17:42:57 PDT
Created attachment 459467 [details]
Patch
Comment 9 Yijia Huang 2022-05-16 17:51:58 PDT
Created attachment 459469 [details]
Patch
Comment 10 Yijia Huang 2022-05-16 18:04:31 PDT
Created attachment 459470 [details]
Patch
Comment 11 Mark Lam 2022-05-16 18:09:15 PDT
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).
Comment 12 Yijia Huang 2022-05-21 13:47:02 PDT
Pull request: https://github.com/WebKit/WebKit/pull/658
Comment 13 Mark Lam 2022-05-21 20:29:20 PDT
Comment on attachment 459470 [details]
Patch

This patch is obsolete.  The current patch is at the PR on GitHub.
Comment 14 Yijia Huang 2022-05-22 13:24:56 PDT
Pull request: https://github.com/WebKit/WebKit/pull/889
Comment 15 Yijia Huang 2022-05-22 13:34:51 PDT
Pull request: https://github.com/WebKit/WebKit/pull/894
Comment 16 Yijia Huang 2022-05-22 13:41:01 PDT
Pull request: https://github.com/WebKit/WebKit/pull/896
Comment 17 Yijia Huang 2022-05-23 20:27:54 PDT
Pull request: https://github.com/WebKit/WebKit/pull/963
Comment 18 Yijia Huang 2022-05-23 22:13:01 PDT
Pull request: https://github.com/WebKit/WebKit/pull/966
Comment 19 Yijia Huang 2022-05-24 00:22:26 PDT
Created attachment 459705 [details]
Patch
Comment 20 Yijia Huang 2022-05-24 14:27:01 PDT
Pull request: https://github.com/WebKit/WebKit/pull/983
Comment 21 EWS 2022-05-28 20:23:52 PDT
Committed r295008 (251103@main): <https://commits.webkit.org/251103@main>

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