Bug 186732 - JITStubRoutineSet wastes 180KB of HashTable capacity on can.com
Summary: JITStubRoutineSet wastes 180KB of HashTable capacity on can.com
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: 2018-06-16 12:27 PDT by Simon Fraser (smfr)
Modified: 2019-04-29 12:21 PDT (History)
13 users (show)

See Also:


Attachments
Patch (9.04 KB, patch)
2019-04-27 00:32 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (9.06 KB, patch)
2019-04-27 11:58 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (9.41 KB, patch)
2019-04-27 11:59 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-highsierra (767.52 KB, application/zip)
2019-04-27 13:08 PDT, Build Bot
no flags Details
Patch (9.41 KB, patch)
2019-04-27 13:22 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.65 KB, patch)
2019-04-27 19:22 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.65 KB, patch)
2019-04-27 19:50 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.63 KB, patch)
2019-04-27 19:58 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.63 KB, patch)
2019-04-28 23:04 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2018-06-16 12:27:29 PDT
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)
Comment 1 Radar WebKit Bug Importer 2018-06-16 12:27:52 PDT
<rdar://problem/41189137>
Comment 2 Simon Fraser (smfr) 2018-06-16 12:36:42 PDT
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)
Comment 3 Yusuke Suzuki 2019-04-26 20:14:31 PDT
In Gmail, it sometimes takes 2MB lol.
Comment 4 Yusuke Suzuki 2019-04-27 00:32:38 PDT
Created attachment 368393 [details]
Patch
Comment 5 Build Bot 2019-04-27 00:35:18 PDT
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.
Comment 6 Yusuke Suzuki 2019-04-27 00:35:52 PDT
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).
Comment 7 Mark Lam 2019-04-27 03:16:48 PDT
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.
Comment 8 Yusuke Suzuki 2019-04-27 11:54:07 PDT
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.
Comment 9 Yusuke Suzuki 2019-04-27 11:58:00 PDT
Created attachment 368406 [details]
Patch
Comment 10 Yusuke Suzuki 2019-04-27 11:59:41 PDT
Created attachment 368408 [details]
Patch
Comment 11 Build Bot 2019-04-27 12:02:33 PDT
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.
Comment 12 Build Bot 2019-04-27 13:08:51 PDT
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.
Comment 13 Build Bot 2019-04-27 13:08:53 PDT
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
Comment 14 Yusuke Suzuki 2019-04-27 13:22:21 PDT
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?
Comment 15 Yusuke Suzuki 2019-04-27 13:22:34 PDT
Created attachment 368411 [details]
Patch
Comment 16 Build Bot 2019-04-27 13:24:09 PDT
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.
Comment 17 Sam Weinig 2019-04-27 14:31:48 PDT
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.
Comment 18 Yusuke Suzuki 2019-04-27 18:58:19 PDT
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.
Comment 19 Yusuke Suzuki 2019-04-27 19:22:16 PDT
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.
Comment 20 Yusuke Suzuki 2019-04-27 19:22:54 PDT
Created attachment 368420 [details]
Patch
Comment 21 Build Bot 2019-04-27 19:24:31 PDT
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.
Comment 22 Yusuke Suzuki 2019-04-27 19:50:20 PDT
Created attachment 368421 [details]
Patch
Comment 23 Build Bot 2019-04-27 19:51:47 PDT
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.
Comment 24 Yusuke Suzuki 2019-04-27 19:58:51 PDT
Created attachment 368422 [details]
Patch
Comment 25 Build Bot 2019-04-27 20:01:59 PDT
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.
Comment 26 Yusuke Suzuki 2019-04-28 13:36:50 PDT
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.
Comment 27 Yusuke Suzuki 2019-04-28 22:51:33 PDT
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.
Comment 28 Yusuke Suzuki 2019-04-28 23:04:44 PDT
Created attachment 368443 [details]
Patch
Comment 29 Build Bot 2019-04-28 23:06:44 PDT
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.
Comment 30 Saam Barati 2019-04-29 11:56:48 PDT
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"
Comment 31 Yusuke Suzuki 2019-04-29 12:01:38 PDT
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.
Comment 32 Yusuke Suzuki 2019-04-29 12:21:12 PDT
Committed r244745: <https://trac.webkit.org/changeset/244745>