Bug 186732 – JITStubRoutineSet wastes 180KB of HashTable capacity on can.com

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

[Simon Fraser (smfr)]

Using tooling from bug 186698, loading can and running "notifyutil -p com.apple.WebKit.dumpHashTableCapacity" shows:

Wasted capacity: 179008 bytes (used 83136 of 262144 bytes, utilization: 31.71%) - 1 allocations
1   0x11606c2a5 WTF::HashTable<unsigned long, WTF::KeyValuePair<unsigned long, JSC::GCAwareJITStubRoutine*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<unsigned long, JSC::GCAwareJITStubRoutine*> >, WTF::IntHash<unsigned long>, WTF::HashMap<unsigned long, JSC::GCAwareJITStubRoutine*, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<JSC::GCAwareJITStubRoutine*> >::KeyValuePairTraits, WTF::HashTraits<unsigned long> >::HashTable()
2   0x11606c285 WTF::HashMap<unsigned long, JSC::GCAwareJITStubRoutine*, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<JSC::GCAwareJITStubRoutine*> >::HashMap()
3   0x116062b75 WTF::HashMap<unsigned long, JSC::GCAwareJITStubRoutine*, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<JSC::GCAwareJITStubRoutine*> >::HashMap()
4   0x116062b49 JSC::JITStubRoutineSet::JITStubRoutineSet()
5   0x116062bb5 JSC::JITStubRoutineSet::JITStubRoutineSet()
6   0x116010051 JSC::Heap::Heap(JSC::VM*, JSC::HeapType)
7   0x1160126b3 JSC::Heap::Heap(JSC::VM*, JSC::HeapType)
8   0x1166b8e72 JSC::VM::VM(JSC::VM::VMType, JSC::HeapType)

[Radar WebKit Bug Importer]

<rdar://problem/41189137>

[Simon Fraser (smfr)]

It's over 800KB on nytimes.com:

Wasted capacity: 848672 bytes (used 199904 of 1048576 bytes, utilization: 19.06%) - 1 allocations
1   0x11606c2a5 WTF::HashTable<unsigned long, WTF::KeyValuePair<unsigned long, JSC::GCAwareJITStubRoutine*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<unsigned long, JSC::GCAwareJITStubRoutine*> >, WTF::IntHash<unsigned long>, WTF::HashMap<unsigned long, JSC::GCAwareJITStubRoutine*, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<JSC::GCAwareJITStubRoutine*> >::KeyValuePairTraits, WTF::HashTraits<unsigned long> >::HashTable()
2   0x11606c285 WTF::HashMap<unsigned long, JSC::GCAwareJITStubRoutine*, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<JSC::GCAwareJITStubRoutine*> >::HashMap()
3   0x116062b75 WTF::HashMap<unsigned long, JSC::GCAwareJITStubRoutine*, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<JSC::GCAwareJITStubRoutine*> >::HashMap()
4   0x116062b49 JSC::JITStubRoutineSet::JITStubRoutineSet()
5   0x116062bb5 JSC::JITStubRoutineSet::JITStubRoutineSet()
6   0x116010051 JSC::Heap::Heap(JSC::VM*, JSC::HeapType)
7   0x1160126b3 JSC::Heap::Heap(JSC::VM*, JSC::HeapType)
8   0x1166b8e72 JSC::VM::VM(JSC::VM::VMType, JSC::HeapType)

[Yusuke Suzuki]

In Gmail, it sometimes takes 2MB lol.

[Yusuke Suzuki]

Created attachment 368393 [details]
Patch

[EWS Watchlist]

Attachment 368393 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:96:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 368393 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:17
> +        performance bots say.

If this becomes a problem, we can introduce some optimization. One thing I come up with is that introducing Eden/Old notion to GCAwareJITStubRoutine and only use Eden routines for Eden collection (as we do for LargeAllocation).

[Mark Lam]

Comment on attachment 368393 [details]
Patch

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

Some drive by comments ...

> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:92
> +    if (m_routines.isEmpty())
>          return;
> -    
> -    iter->value->m_mayBeExecuting = true;
> +    if (m_startOfRange > address || address >= m_endOfRange)
> +        return;

By doing the routine range check in the fast mark(), you can remove these checks here and ASSERT(!m_routines.isEmpty()) and ASSERT(isJITPC(bitwise_cast<void*>(address))) here instead.

> Source/JavaScriptCore/heap/JITStubRoutineSet.h:55
> +        if (!isJITPC(bitwise_cast<void*>(address)))

Just check (m_startOfRange > address || address >= m_endOfRange) here instead.  That range should strictly be a subset of the isJITPC() range.

You can ASSERT(isJITPC(...)) in markSlow() as a sanity check.

[Yusuke Suzuki]

Comment on attachment 368393 [details]
Patch

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

>> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:92
>> +        return;
> 
> By doing the routine range check in the fast mark(), you can remove these checks here and ASSERT(!m_routines.isEmpty()) and ASSERT(isJITPC(bitwise_cast<void*>(address))) here instead.

Right! Fixed, that's nice.

>> Source/JavaScriptCore/heap/JITStubRoutineSet.h:55
>> +        if (!isJITPC(bitwise_cast<void*>(address)))
> 
> Just check (m_startOfRange > address || address >= m_endOfRange) here instead.  That range should strictly be a subset of the isJITPC() range.
> 
> You can ASSERT(isJITPC(...)) in markSlow() as a sanity check.

Fixed.

[Yusuke Suzuki]

Created attachment 368406 [details]
Patch

[Yusuke Suzuki]

Created attachment 368408 [details]
Patch

[EWS Watchlist]

Attachment 368408 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:94:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 368408 [details]
Patch

Attachment 368408 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12017615

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 368410 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

Comment on attachment 368408 [details]
Patch

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

> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:136
>          if (!routine->m_mayBeExecuting)

It is not directly related to this change, but I'm not sure why this is correct. Why don't we need to mark cells when stub routine is not executed right now but it is live?

[Yusuke Suzuki]

Created attachment 368411 [details]
Patch

[EWS Watchlist]

Attachment 368411 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:94:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 368393 [details]
Patch

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

> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:61
> +        routine,

The trailing , is not needed here.

> Source/JavaScriptCore/heap/JITStubRoutineSet.h:76
> +    uintptr_t m_startOfRange { 0 };
> +    uintptr_t m_endOfRange { 0 };

Wouldn't buy you much, but you could use WTF::Range<uintptr_t> here if you wanted.

[Yusuke Suzuki]

Comment on attachment 368393 [details]
Patch

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

>> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:61
>> +        routine,
> 
> The trailing , is not needed here.

Dropped.

>> Source/JavaScriptCore/heap/JITStubRoutineSet.h:76
>> +    uintptr_t m_endOfRange { 0 };
> 
> Wouldn't buy you much, but you could use WTF::Range<uintptr_t> here if you wanted.

Sounds fine! fixed.

[Yusuke Suzuki]

Comment on attachment 368408 [details]
Patch

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

>> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:136
>>          if (!routine->m_mayBeExecuting)
> 
> It is not directly related to this change, but I'm not sure why this is correct. Why don't we need to mark cells when stub routine is not executed right now but it is live?

It is correct as long as we maintain JITStubRoutines by visitWeak.

[Yusuke Suzuki]

Created attachment 368420 [details]
Patch

[EWS Watchlist]

Attachment 368420 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 368421 [details]
Patch

[EWS Watchlist]

Attachment 368421 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 368422 [details]
Patch

[EWS Watchlist]

Attachment 368422 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 368422 [details]
Patch

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

> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:99
> +                routine.routine->m_mayBeExecuting = true;

I think we should track this information only when,

1. stub requires marking
2. stub is jettisoned

We could make the size of array small I think. I'll do that.

[Yusuke Suzuki]

Comment on attachment 368422 [details]
Patch

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

>> Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:99
>> +                routine.routine->m_mayBeExecuting = true;
> 
> I think we should track this information only when,
> 
> 1. stub requires marking
> 2. stub is jettisoned
> 
> We could make the size of array small I think. I'll do that.

For now, I'll start with this simple patch.

[Yusuke Suzuki]

Created attachment 368443 [details]
Patch

[EWS Watchlist]

Attachment 368443 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/JITStubRoutineSet.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 368443 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Our current mechanism of JITStubRoutineSet is too memory consuming. Basically we have HashMap<uintptr_t, StubRoutine*> and register

"is too memory consuming" => "consumes more memory than needed".

> Source/JavaScriptCore/ChangeLog:10
> +        In Gmail, we see ~2MB table size.

"see" => "see a"

[Yusuke Suzuki]

Comment on attachment 368443 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:8
>> +        Our current mechanism of JITStubRoutineSet is too memory consuming. Basically we have HashMap<uintptr_t, StubRoutine*> and register
> 
> "is too memory consuming" => "consumes more memory than needed".

Fixed.

>> Source/JavaScriptCore/ChangeLog:10
>> +        In Gmail, we see ~2MB table size.
> 
> "see" => "see a"

Fixed.

[Yusuke Suzuki]

Committed r244745: <https://trac.webkit.org/changeset/244745>