Bug 207827 - [JSC] Shrink Structure
Summary: [JSC] Shrink Structure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-16 02:50 PST by Yusuke Suzuki
Modified: 2020-04-03 08:59 PDT (History)
18 users (show)

See Also:


Attachments
Patch (50.22 KB, patch)
2020-02-17 11:45 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (50.22 KB, patch)
2020-02-17 11:46 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (80.99 KB, patch)
2020-02-17 18:15 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (82.40 KB, patch)
2020-02-17 20:26 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (97.84 KB, patch)
2020-02-21 18:56 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (104.91 KB, patch)
2020-02-21 21:58 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (104.94 KB, patch)
2020-02-21 22:30 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (109.29 KB, patch)
2020-02-21 23:11 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (109.29 KB, patch)
2020-02-22 00:55 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (127.97 KB, patch)
2020-02-23 02:48 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (122.71 KB, patch)
2020-02-23 03:28 PST, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-02-16 02:50:58 PST
Let's do super aggressive thing and gets small Structure.
Comment 1 Yusuke Suzuki 2020-02-17 11:45:27 PST
Created attachment 390953 [details]
Patch
Comment 2 Yusuke Suzuki 2020-02-17 11:46:08 PST
Created attachment 390954 [details]
Patch
Comment 3 Yusuke Suzuki 2020-02-17 18:15:18 PST
Created attachment 391014 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2020-02-17 18:15:34 PST
<rdar://problem/59534793>
Comment 5 Yusuke Suzuki 2020-02-17 19:56:30 PST
The failing test is known that it is relying on pointer orders. So, this is flaky.
Comment 6 Yusuke Suzuki 2020-02-17 20:26:01 PST
Created attachment 391023 [details]
Patch
Comment 7 Saam Barati 2020-02-17 22:12:57 PST
Comment on attachment 391023 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +            2. Instead of using bloom-filter of pointers for seen-properties, we should use bloom-filter of hash-codes.

You sure this is ok? I specifically didn’t do this because it made the bloom filter way less effective (IIRC, I measured rule-out rate running Speedo2 and browsing the web)
Comment 8 Mark Lam 2020-02-17 22:32:44 PST
Comment on attachment 391023 [details]
Patch

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

> Source/JavaScriptCore/runtime/Structure.h:501
>      bool hasInlineStorage() const
>      {
> -        return !!m_inlineCapacity;
> -    }
> -    unsigned inlineCapacity() const
> -    {
> -        return m_inlineCapacity;
> +        return !!inlineCapacity();
>      }

nit: This can be a 1 liner too for compactness.

> Source/JavaScriptCore/runtime/Structure.h:636
> +#if CPU(LITTLE_ENDIAN)
> +    static ptrdiff_t offsetOfInlineCapacity()
>      {
> -        return OBJECT_OFFSETOF(Structure, m_inlineCapacity);
> +        return OBJECT_OFFSETOF(Structure, m_propertyHashAndInlineCapacity);
>      }
> +#endif

Can you just provide the BIG_ENDIAN version as well?  The MIPS folks would probably appreciate that.  I'm thinking OBJECT_OFFSETOF(Structure, m_propertyHashAndInlineCapacity) + 3?

> Source/JavaScriptCore/runtime/Structure.h:763
> +    DEFINE_BITFIELD(unsigned, transitionPropertyAttributes, TransitionPropertyAttributes, 8, 6);

Is this enough bits?  Shouldn't this be 10 bits?

> Source/JavaScriptCore/runtime/Structure.h:776
>      static_assert(s_bitWidthOfTransitionPropertyAttributes <= sizeof(TransitionPropertyAttributes) * 8);

Why <= here?  Why not ==?

On the other hand, if I'm correct that TransitionPropertyAttributes needs to be 10 bits, then this static_assert should stay unchanged.

> Source/JavaScriptCore/runtime/Structure.h:894
> +    // Should be accessed through ensurePropertyTable(). During GC, it may be set to 0 by another thread.
> +    // During a Heap Snapshot GC we avoid clearing the table so it is safe to use.

This comment used to be associated with m_propertyTableUnsafe.  Now, this association is not totally clear.  I'm also not sure if the comment is stale, but at minimum, we should give it some context, like:

The property table pointer should be accessed through ensurePropertyTable(). During GC, ...

> Source/JavaScriptCore/runtime/Structure.h:916
> +    PackedRefPtr<UniquedStringImpl> m_transitionPropertyName;

This is tricky.  At first, I was wondering how this saves any space.  And then I realized that StructureTransitionTable is byte aligned, and only contains a PackedPtr.  Can you static_assert that PackedRefPtr and StructureTransitionTable are byte aligned?  I think that this is essential in order for m_bitField to get packed in with them to only use 32 bytes.

> Source/JavaScriptCore/runtime/StructureInlines.h:149
> +    unsigned hashCode = propertyName.uid()->symbolAwareHash();
> +    ASSERT(hashCode); // StringImpl hash never gets zero.
> +    if ((m_seenProperties & hashCode) != hashCode)

It would be good to factor this into an inline function ruleOutUnseenPropertyHash() and locate it near the addSeenPropertyHash() below.  The reason for doing this is so that the 2 are co-located, and it becomes easier to see that you're implementing a bloom filter here.  Right now, in this review, it's easy to see that, but it becomes less clear for someone reading this code later who does not have the benefit of reading this diff.

> Source/JavaScriptCore/runtime/StructureInlines.h:499
> +    unsigned hashCode = rep->existingSymbolAwareHash();
> +    setPropertyHash(propertyHash() ^ hashCode);
> +    m_seenProperties = m_seenProperties | hashCode;

Let's put this in an inline function addSeenPropertyHash() and locate it near the ruleOutUnseenPropertyHash() function above.

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:61
> +inline constexpr uint8_t toAttributes(NonPropertyTransition transition)
>  {
>      return static_cast<unsigned>(transition) + FirstInternalAttribute;
>  }

How can this be correct?  FirstInternalAttribute is 1 << 6 i.e. 6 bits.  There are 11 NonPropertyTransitions i.e. 4 bits.  Total unique bits = 10.  Aren't we potentially losing the top 2 bits here?
Comment 9 Mark Lam 2020-02-17 22:40:27 PST
Comment on attachment 391023 [details]
Patch

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

r=me with other suggestions, and if there's no perf regression.

>> Source/JavaScriptCore/runtime/StructureTransitionTable.h:61
>>  }
> 
> How can this be correct?  FirstInternalAttribute is 1 << 6 i.e. 6 bits.  There are 11 NonPropertyTransitions i.e. 4 bits.  Total unique bits = 10.  Aren't we potentially losing the top 2 bits here?

I was wrong.  Yusuke pointed out that we're computing NonPropertyTransition + 1<<6, not NonPropertyTransition << 6.
Comment 10 Mark Lam 2020-02-17 23:27:07 PST
Comment on attachment 391023 [details]
Patch

Clearing the r+ for now. Yusuke and I have spotted some concurrency issues.
Comment 11 Yusuke Suzuki 2020-02-21 18:56:00 PST
Created attachment 391443 [details]
Patch
Comment 12 Yusuke Suzuki 2020-02-21 21:50:09 PST
Comment on attachment 391023 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:14
>> +            2. Instead of using bloom-filter of pointers for seen-properties, we should use bloom-filter of hash-codes.
> 
> You sure this is ok? I specifically didn’t do this because it made the bloom filter way less effective (IIRC, I measured rule-out rate running Speedo2 and browsing the web)

For now, using pointer based filter.

>> Source/JavaScriptCore/runtime/Structure.h:501
>>      }
> 
> nit: This can be a 1 liner too for compactness.

Fixed.

>> Source/JavaScriptCore/runtime/Structure.h:636
>> +#endif
> 
> Can you just provide the BIG_ENDIAN version as well?  The MIPS folks would probably appreciate that.  I'm thinking OBJECT_OFFSETOF(Structure, m_propertyHashAndInlineCapacity) + 3?

I think this is not necessary. This function is used only from JIT, and our JIT only supports little-endian environment.

>> Source/JavaScriptCore/runtime/Structure.h:763
>> +    DEFINE_BITFIELD(unsigned, transitionPropertyAttributes, TransitionPropertyAttributes, 8, 6);
> 
> Is this enough bits?  Shouldn't this be 10 bits?

Discussed with Mark, it is enough.

>> Source/JavaScriptCore/runtime/Structure.h:776
>>      static_assert(s_bitWidthOfTransitionPropertyAttributes <= sizeof(TransitionPropertyAttributes) * 8);
> 
> Why <= here?  Why not ==?
> 
> On the other hand, if I'm correct that TransitionPropertyAttributes needs to be 10 bits, then this static_assert should stay unchanged.

So long as it is smaller than TransitionPropertyAttributes's size, it is OK.

>> Source/JavaScriptCore/runtime/Structure.h:894
>> +    // During a Heap Snapshot GC we avoid clearing the table so it is safe to use.
> 
> This comment used to be associated with m_propertyTableUnsafe.  Now, this association is not totally clear.  I'm also not sure if the comment is stale, but at minimum, we should give it some context, like:
> 
> The property table pointer should be accessed through ensurePropertyTable(). During GC, ...

Fixed.

>> Source/JavaScriptCore/runtime/Structure.h:916
>> +    PackedRefPtr<UniquedStringImpl> m_transitionPropertyName;
> 
> This is tricky.  At first, I was wondering how this saves any space.  And then I realized that StructureTransitionTable is byte aligned, and only contains a PackedPtr.  Can you static_assert that PackedRefPtr and StructureTransitionTable are byte aligned?  I think that this is essential in order for m_bitField to get packed in with them to only use 32 bytes.

Added.

>> Source/JavaScriptCore/runtime/StructureInlines.h:149
>> +    if ((m_seenProperties & hashCode) != hashCode)
> 
> It would be good to factor this into an inline function ruleOutUnseenPropertyHash() and locate it near the addSeenPropertyHash() below.  The reason for doing this is so that the 2 are co-located, and it becomes easier to see that you're implementing a bloom filter here.  Right now, in this review, it's easy to see that, but it becomes less clear for someone reading this code later who does not have the benefit of reading this diff.

Fixed.

>> Source/JavaScriptCore/runtime/StructureInlines.h:499
>> +    m_seenProperties = m_seenProperties | hashCode;
> 
> Let's put this in an inline function addSeenPropertyHash() and locate it near the ruleOutUnseenPropertyHash() function above.

I've added addPropertyHashAndSeenProperty, which take hash and property, and update propertyHash and seenProperties.
Comment 13 Yusuke Suzuki 2020-02-21 21:58:40 PST
Created attachment 391452 [details]
Patch
Comment 14 Yusuke Suzuki 2020-02-21 22:30:06 PST
Created attachment 391453 [details]
Patch
Comment 15 Robin Morisset 2020-02-21 22:43:40 PST
Comment on attachment 391452 [details]
Patch

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

I only some of the files for now

> Source/JavaScriptCore/ChangeLog:14
> +            2. Instead of using bloom-filter of pointers for seen-properties, we should use bloom-filter of hash-codes.

Please update changelog since you apparently removed that change

> Source/JavaScriptCore/ChangeLog:29
> +        Combining all of the above techniques and getting 16 bytes.

nit: and getting => gets us

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12937
> +    m_jit.loadPtr(CCallHelpers::Address(structureGPR, Structure::offsetOfClassInfo()), scratch1GPR);

This pattern to load the classInfo of a structure seems to recur often enough to justify a small helper in AssemblyHelpers.h, just to help readability.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6094
> +                LValue previousOrRareData = m_out.bitAnd(m_out.constIntPtr(Structure::previousOrRareDataMask), m_out.loadPtr(structure, m_heaps.Structure_previousOrRareData));

Structure::previousOrRareDataMask was protected by #if USE(JSVALUE64) before. Which one was correct?
This is another example of a pattern which would be less error prone if it was written only once in a helper function.
Also please test this patch on a 32-bit platform if you have not done so yet. The code paths are sufficiently different on 32 and 64 bits to be a likely source of bugs.

> Source/JavaScriptCore/runtime/ConcurrentJSLock.h:37
>  static_assert(sizeof(ConcurrentJSLock) == 1, "Regardless of status of concurrent JS flag, size of ConurrentJSLock is always one byte.");

unrelated typo: Conurrent => Concurrent

> Source/JavaScriptCore/runtime/Structure.cpp:367
> +    Vector<std::pair<Structure*, UniquedStringImpl*>, 8> structures;

I am confused by this change.
1) The name no longer match
2) More importantly I can not find any access to the second element

> Source/JavaScriptCore/runtime/Structure.cpp:709
>      // This must always return a property table. It can't return null.

This comment is confusing. Does "This" refer to the next line (clearly wrong then) or to the overall function (In this case it would be much clearer to s/This/This function/)

> Source/JavaScriptCore/runtime/Structure.cpp:1138
>          // That's fine, because then the barrier will fire and we will scan this again.

This comment refer to a barrier, but you modify the next line to use appendUnbarriered, is it normal?

> Source/JavaScriptCore/runtime/WriteBarrier.h:251
> +using UnsafeCellPointer = uintptr_t;

Where is this used?

> Source/WTF/wtf/CompactPointerTuple.h:34
>  // The goal of this class is folding a pointer and 1 byte value into 8 bytes in both 32bit and 64bit architectures.

Please update comment since this class can now store 2 bytes + a pointer.

> Source/WTF/wtf/CompactPointerTuple.h:59
> +    static constexpr uint64_t encodeType(Type type)

This could be a private method.

> Source/WTF/wtf/CompactPointerTuple.h:63
> +    static constexpr Type decodeType(uint64_t value)

Ditto.

> Source/WTF/wtf/text/SymbolImpl.h:44
> +    unsigned hashForSymbol() const { return m_hashForSymbol >> s_flagCount; }

The name of "m_hashForSymbol" is not great if it stores more than just hashForSymbol
Comment 16 Yusuke Suzuki 2020-02-21 23:01:41 PST
Comment on attachment 391452 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:14
>> +            2. Instead of using bloom-filter of pointers for seen-properties, we should use bloom-filter of hash-codes.
> 
> Please update changelog since you apparently removed that change

Fixed.

>> Source/JavaScriptCore/ChangeLog:29
>> +        Combining all of the above techniques and getting 16 bytes.
> 
> nit: and getting => gets us

Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12937
>> +    m_jit.loadPtr(CCallHelpers::Address(structureGPR, Structure::offsetOfClassInfo()), scratch1GPR);
> 
> This pattern to load the classInfo of a structure seems to recur often enough to justify a small helper in AssemblyHelpers.h, just to help readability.

Sounds nice. Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6094
>> +                LValue previousOrRareData = m_out.bitAnd(m_out.constIntPtr(Structure::previousOrRareDataMask), m_out.loadPtr(structure, m_heaps.Structure_previousOrRareData));
> 
> Structure::previousOrRareDataMask was protected by #if USE(JSVALUE64) before. Which one was correct?
> This is another example of a pattern which would be less error prone if it was written only once in a helper function.
> Also please test this patch on a 32-bit platform if you have not done so yet. The code paths are sufficiently different on 32 and 64 bits to be a likely source of bugs.

FTL only supports 64bit, and all the other code in FTL strongly assumes 64bit right now. So, I think this is OK.

>> Source/JavaScriptCore/runtime/ConcurrentJSLock.h:37
>>  static_assert(sizeof(ConcurrentJSLock) == 1, "Regardless of status of concurrent JS flag, size of ConurrentJSLock is always one byte.");
> 
> unrelated typo: Conurrent => Concurrent

Fixed.

>> Source/JavaScriptCore/runtime/Structure.cpp:367
>> +    Vector<std::pair<Structure*, UniquedStringImpl*>, 8> structures;
> 
> I am confused by this change.
> 1) The name no longer match
> 2) More importantly I can not find any access to the second element

1) I'll update
2) UniquedStringImpl* is used in Structure::forEachPropertyConcurrently. This is added for that function because forEachPropertyConcurrently can be called concurrently. In that case, we should collect UniquedStringImpl* when structure lock is held.
For consistency, I'll use this UniquedStringImpl* in this function too.

>> Source/JavaScriptCore/runtime/Structure.cpp:709
>>      // This must always return a property table. It can't return null.
> 
> This comment is confusing. Does "This" refer to the next line (clearly wrong then) or to the overall function (In this case it would be much clearer to s/This/This function/)

This is "This function". Updated.

>> Source/JavaScriptCore/runtime/Structure.cpp:1138
>>          // That's fine, because then the barrier will fire and we will scan this again.
> 
> This comment refer to a barrier, but you modify the next line to use appendUnbarriered, is it normal?

Yes. visitor.appendUnbarriered is used when we do not have WriteBarrier<> instance for this field, (Unbarrier => no WriteBarrier<> field).
But when setting propertyTableUnsafeOrNull's field, we are emitting a barrier (vm.heap.writeBarrier), so barrier exists.

>> Source/JavaScriptCore/runtime/WriteBarrier.h:251
>> +using UnsafeCellPointer = uintptr_t;
> 
> Where is this used?

This was originally used, but it is no longer used, removed.

>> Source/WTF/wtf/CompactPointerTuple.h:34
>>  // The goal of this class is folding a pointer and 1 byte value into 8 bytes in both 32bit and 64bit architectures.
> 
> Please update comment since this class can now store 2 bytes + a pointer.

Fixed.

>> Source/WTF/wtf/CompactPointerTuple.h:59
>> +    static constexpr uint64_t encodeType(Type type)
> 
> This could be a private method.

Right, fixed.

>> Source/WTF/wtf/CompactPointerTuple.h:63
>> +    static constexpr Type decodeType(uint64_t value)
> 
> Ditto.

Fixed.

>> Source/WTF/wtf/text/SymbolImpl.h:44
>> +    unsigned hashForSymbol() const { return m_hashForSymbol >> s_flagCount; }
> 
> The name of "m_hashForSymbol" is not great if it stores more than just hashForSymbol

This only holds hash-for-symbol actually.  This field does not include any flags. It is shifted with flagCount because we would like to adjust effective-bits-of-hash with StringImpl (which only offers 24bits hash).
m_hashForSymbolShiftedWithFlagCount would be the precise name.
Comment 17 Yusuke Suzuki 2020-02-21 23:11:12 PST
Created attachment 391454 [details]
Patch
Comment 18 Yusuke Suzuki 2020-02-22 00:55:03 PST
Created attachment 391456 [details]
Patch
Comment 19 Yusuke Suzuki 2020-02-22 11:45:29 PST
Comment on attachment 391456 [details]
Patch

I'll do a bit more.
Comment 20 Yusuke Suzuki 2020-02-23 02:48:03 PST
Created attachment 391486 [details]
Patch
Comment 21 Yusuke Suzuki 2020-02-23 03:28:38 PST
Created attachment 391488 [details]
Patch
Comment 22 Sam Weinig 2020-02-23 08:48:29 PST
Comment on attachment 391488 [details]
Patch

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

> Source/WTF/wtf/CompactPointerTuple.h:52
> +    static_assert(WTF_OS_CONSTANT_EFFECTIVE_ADDRESS_WIDTH <= 48);

This should be written as static_assert(OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) <= 48); if possible.
Comment 23 Saam Barati 2020-02-23 18:18:39 PST
Comment on attachment 391488 [details]
Patch

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

nice. r=me

Mostly minor nits.

Can you test this on arm64_32 to make sure nothing broke there?

Might also be worth filing a bug for Igalia folks to look into 32-bit on their architectures

> Source/JavaScriptCore/ChangeLog:8
> +        This patch shrinks sizeof(Structure) from 112 to 96 (16 bytes) in 64 bit architectures.

it's not 64-bit architectures, its architectures with 64-bit pointers. (Since arm64_32 does not do this)

> Source/JavaScriptCore/ChangeLog:13
> +            1. StringHasher is always generating 24 bits hashes so far. By leveraging this fact,
> +               we store hash code in Structure in 3 bytes (24 bits) for property-hash and seen-property-hash.

the code isn't actually doing this. AFAICT, it uses 48 bits for seen properties, and uses 16 bits to accumulate hash, and then uses the 64-bit combination of those things as the "seen property hash"

(On a side note, I wonder if we really even need the seen property hash anymore, since the bloom filter might be sufficient. But I guess if we have the spare bits, it doesn't hurt to keep it.)

> Source/JavaScriptCore/ChangeLog:20
> +               We are setting m_cachedPrototypeChain only if Structure is for JSObject. Clearing happens only if it is not

"are" => "were".
"happens" => "happened"
"it is not" => "it was not"

> Source/JavaScriptCore/ChangeLog:25
> +        Combining all of the above techniques and gets us 16 bytes.

"and gets us" => "saves us"

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6094
> +                LValue cachedPrototypeChainOrRareData = m_out.bitAnd(m_out.constIntPtr(Structure::cachedPrototypeChainOrRareDataMask), m_out.loadPtr(structure, m_heaps.Structure_cachedPrototypeChainOrRareData));

only do the and if address64?  (and a few other places too in this file)

You could even have a helper that masks out on address64 architectures

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1523
> +#if CPU(ADDRESS64) // Not USE(JSVALUE64). Structure::m_classInfo's implementation is changed by size of pointer.

not sure this comment is needed given you do this everywhere without this comment

> Source/JavaScriptCore/runtime/JSObjectInlines.h:219
> +        [&] (const GCSafeConcurrentJSCellLocker&, PropertyOffset offset, PropertyOffset newMaxOffset) {

nit: also fix style like you did elsewhere?

> Source/JavaScriptCore/runtime/Structure.h:920
> +    // Structure is one of the most frequently allocated data structure. Moreover, Structure tends to be alive long!

"alive long" => "alive a long time"

> Source/JavaScriptCore/runtime/Structure.h:929
> +    //  Other pairs works well. We carefully put assertions to setters, analyze access patterns and pick appropriate pairs in Structure fields.

nice

> Source/JavaScriptCore/runtime/StructureInlines.h:162
> +inline bool Structure::ruleOutUnseenPropertyHash(UniquedStringImpl* uid) const

this should be called `ruleOutUnseenProperty`. It has nothing to do with hashes.

> Source/JavaScriptCore/runtime/StructureInlines.h:177
> +#if CPU(ADDRESS64)
> +    return bitwise_cast<uintptr_t>(m_propertyHashAndSeenProperties.pointer());
> +#else
> +    return m_seenProperties;
> +#endif

minor style nit: I'd make this just return a TinyBloomFilter, and the code inside ruleOut can just call ruleOut.

> Source/WTF/wtf/CompactPointerTuple.h:57
> +        return 48 / 8;

should we give 48 a name instead of using it directly everywhere?

Maybe maxNumberOfBitsInPointer?

> Source/WTF/wtf/CompactRefPtrTuple.h:34
> +class CompactRefPtrTuple final {

nice

> LayoutTests/ChangeLog:9
> +        This test is half-broken since it relies on HashMap's order implicitly.
> +        We changed SymbolImpl's hash code, so it makes the result different.

can you file a bug so we can make the test not reliant one this?
Comment 24 Yusuke Suzuki 2020-02-23 23:52:39 PST
Comment on attachment 391488 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:8
>> +        This patch shrinks sizeof(Structure) from 112 to 96 (16 bytes) in 64 bit architectures.
> 
> it's not 64-bit architectures, its architectures with 64-bit pointers. (Since arm64_32 does not do this)

Fixed.

>> Source/JavaScriptCore/ChangeLog:13
>> +               we store hash code in Structure in 3 bytes (24 bits) for property-hash and seen-property-hash.
> 
> the code isn't actually doing this. AFAICT, it uses 48 bits for seen properties, and uses 16 bits to accumulate hash, and then uses the 64-bit combination of those things as the "seen property hash"
> 
> (On a side note, I wonder if we really even need the seen property hash anymore, since the bloom filter might be sufficient. But I guess if we have the spare bits, it doesn't hurt to keep it.)

Yes. I've just removed this (1).

>> Source/JavaScriptCore/ChangeLog:20
>> +               We are setting m_cachedPrototypeChain only if Structure is for JSObject. Clearing happens only if it is not
> 
> "are" => "were".
> "happens" => "happened"
> "it is not" => "it was not"

Fixed.

>> Source/JavaScriptCore/ChangeLog:25
>> +        Combining all of the above techniques and gets us 16 bytes.
> 
> "and gets us" => "saves us"

Fixed.

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:1523
>> +#if CPU(ADDRESS64) // Not USE(JSVALUE64). Structure::m_classInfo's implementation is changed by size of pointer.
> 
> not sure this comment is needed given you do this everywhere without this comment

Removed.

>> Source/JavaScriptCore/runtime/JSObjectInlines.h:219
>> +        [&] (const GCSafeConcurrentJSCellLocker&, PropertyOffset offset, PropertyOffset newMaxOffset) {
> 
> nit: also fix style like you did elsewhere?

Fixed.

>> Source/JavaScriptCore/runtime/Structure.h:920
>> +    // Structure is one of the most frequently allocated data structure. Moreover, Structure tends to be alive long!
> 
> "alive long" => "alive a long time"

Fixed.

>> Source/JavaScriptCore/runtime/StructureInlines.h:162
>> +inline bool Structure::ruleOutUnseenPropertyHash(UniquedStringImpl* uid) const
> 
> this should be called `ruleOutUnseenProperty`. It has nothing to do with hashes.

Fixed.

>> Source/JavaScriptCore/runtime/StructureInlines.h:177
>> +#endif
> 
> minor style nit: I'd make this just return a TinyBloomFilter, and the code inside ruleOut can just call ruleOut.

Fixed.

>> Source/WTF/wtf/CompactPointerTuple.h:52
>> +    static_assert(WTF_OS_CONSTANT_EFFECTIVE_ADDRESS_WIDTH <= 48);
> 
> This should be written as static_assert(OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) <= 48); if possible.

Fixed.

>> Source/WTF/wtf/CompactPointerTuple.h:57
>> +        return 48 / 8;
> 
> should we give 48 a name instead of using it directly everywhere?
> 
> Maybe maxNumberOfBitsInPointer?

Fixed.

>> LayoutTests/ChangeLog:9
>> +        We changed SymbolImpl's hash code, so it makes the result different.
> 
> can you file a bug so we can make the test not reliant one this?

Filed https://bugs.webkit.org/show_bug.cgi?id=208118
Comment 25 Yusuke Suzuki 2020-02-24 00:35:58 PST
Comment on attachment 391488 [details]
Patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6094
>> +                LValue cachedPrototypeChainOrRareData = m_out.bitAnd(m_out.constIntPtr(Structure::cachedPrototypeChainOrRareDataMask), m_out.loadPtr(structure, m_heaps.Structure_cachedPrototypeChainOrRareData));
> 
> only do the and if address64?  (and a few other places too in this file)
> 
> You could even have a helper that masks out on address64 architectures

I think the current FTL does not assume non-address-64 environment. FTL supports address64 environment and does not support other environments.

Just adding this helper is not enough to make FTL work with ARM64_32.
For example, so many places are using Int64 / pointerType() in the same meaning.
For now, I don't think we should assume non-address-64 case in FTL to make code simple.
Comment 26 Yusuke Suzuki 2020-02-24 00:38:56 PST
Committed r257201: <https://trac.webkit.org/changeset/257201>
Comment 27 Yusuke Suzuki 2020-02-24 09:42:25 PST
Committed r257212: <https://trac.webkit.org/changeset/257212>
Comment 28 Caio Lima 2020-02-24 09:46:46 PST
Comment on attachment 391488 [details]
Patch

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

> Source/JavaScriptCore/runtime/Structure.h:754
> +        m_maxOffsetAndTransitionPropertyName.setPointer(transitionPropertyName);

We are missing 32-bit case here. This is causing build failures on ports where `CPU(ADDRESS64)` is false.
Comment 29 Caio Lima 2020-02-24 09:59:41 PST
(In reply to Yusuke Suzuki from comment #27)
> Committed r257212: <https://trac.webkit.org/changeset/257212>

This fix what I mentioned. Thank you.
Comment 30 Yusuke Suzuki 2020-02-24 13:18:43 PST
Committed r257237: <https://trac.webkit.org/changeset/257237>
Comment 31 Yusuke Suzuki 2020-02-24 14:46:13 PST
Committed r257257: <https://trac.webkit.org/changeset/257257>
Comment 32 Yusuke Suzuki 2020-04-03 08:59:15 PDT
Committed r259463: <https://trac.webkit.org/changeset/259463>