Bug 176010 – [DFG] Optimize WeakMap::get by adding intrinsic and fixup

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

[Yusuke Suzuki]

It turns out that Ember.js non-debug takes 1% for WeakMap::get()!

[Yusuke Suzuki]

Created attachment 319153 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:439:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 33 files


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

[Yusuke Suzuki]

Created attachment 319154 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:439:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 34 files


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

[Yusuke Suzuki]

Created attachment 319155 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:439:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 34 files


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

[Yusuke Suzuki]

Created attachment 319157 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:439:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 34 files


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

[Yusuke Suzuki]

Created attachment 319355 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:439:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 38 files


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

[Darin Adler]

Comment on attachment 319355 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:15
> +        1. We use fastGet to inline WeakMap#get operation in the native function.
> +        Since this native function itself is very small, we should inline HashMap#get
> +        entirely in this function.

Can we call this inlinedGet or inlineGet?

[Yusuke Suzuki]

Comment on attachment 319355 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:15
>> +        entirely in this function.
> 
> Can we call this inlinedGet or inlineGet?

OK, renamed.

[Yusuke Suzuki]

Created attachment 319839 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:437:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 42 files


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

[Yusuke Suzuki]

Comment on attachment 319839 [details]
Patch

Ping?

[Darin Adler]

Comment on attachment 319839 [details]
Patch

I’d love to review but I am likely not familiar enough with JIT type speculation details to know if it is complete. In other words, I can’t really review the part of this in the bytecode, dfg, and jit directories. The part in the runtime and wtf directories look great to me (as well as the changes to rename fastGet to inlineGet), so consider those reviewed.

[Filip Pizlo]

Comment on attachment 319839 [details]
Patch

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

R=me other than the AI issue.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1077
> +        forNode(node).makeBytecodeTop();

Why bytecode top?

Isn’t this heap top?

(It cannot return empty)

[Yusuke Suzuki]

Comment on attachment 319839 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1077
>> +        forNode(node).makeBytecodeTop();
> 
> Why bytecode top?
> 
> Isn’t this heap top?
> 
> (It cannot return empty)

Oops, right. Fixed.

[Yusuke Suzuki]

Committed r221854: <http://trac.webkit.org/changeset/221854>

[Ryan Haddad]

(In reply to Yusuke Suzuki from comment #19)
> Committed r221854: <http://trac.webkit.org/changeset/221854>
The test added with this change is failing on 32-bit JSC bots:
https://build.webkit.org/builders/Apple%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1812
https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/1467

[Saam Barati]

Comment on attachment 319839 [details]
Patch

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

LGTM too. Just a couple of comments

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2988
> +        Node* hash = addToGraph(MapHash, key);

WeakMap uses the same hash as Map/Set? I guess we're relying on WTF::HashMap using Wang's int64 hash for objects? I feel like we should perhaps provide a hash inside WeakMapBase to make this explicit.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10361
> +    m_jit.exceptionCheck();

is this actually possible?

[Saam Barati]

Comment on attachment 319839 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2988
>> +        Node* hash = addToGraph(MapHash, key);
> 
> WeakMap uses the same hash as Map/Set? I guess we're relying on WTF::HashMap using Wang's int64 hash for objects? I feel like we should perhaps provide a hash inside WeakMapBase to make this explicit.

To expand a bit more on this, I want the freedom to change what MapHash does, along with the hash we use for JSMap/JSSet without having to change things in WTF. I was actually thinking of looking into this again recently. I think we can probably find something cheaper than Wang's int64 hash that's still good enough for JSMap/JSSet's needs. That's why I think we should explicitly pass a traits that defines a hash for WeakMapBase.

[Ryan Haddad]

Reverted r221854 for reason:

The test added with this change fails on 32-bit JSC bots.

Committed r221876: <http://trac.webkit.org/changeset/221876>

[Yusuke Suzuki]

Comment on attachment 319839 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2988
>>> +        Node* hash = addToGraph(MapHash, key);
>> 
>> WeakMap uses the same hash as Map/Set? I guess we're relying on WTF::HashMap using Wang's int64 hash for objects? I feel like we should perhaps provide a hash inside WeakMapBase to make this explicit.
> 
> To expand a bit more on this, I want the freedom to change what MapHash does, along with the hash we use for JSMap/JSSet without having to change things in WTF. I was actually thinking of looking into this again recently. I think we can probably find something cheaper than Wang's int64 hash that's still good enough for JSMap/JSSet's needs. That's why I think we should explicitly pass a traits that defines a hash for WeakMapBase.

Sounds nice. Yeah, we are currently using wang's hash function for HashMap. So functionality itself is ok.
I'll add an explicit HashFunction for WeakMap/Set's HashMap.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10361
>> +    m_jit.exceptionCheck();
> 
> is this actually possible?

We can drop this check. Dropped in a relanding patch.

[Yusuke Suzuki]

(In reply to Ryan Haddad from comment #23)
> Reverted r221854 for reason:
> 
> The test added with this change fails on 32-bit JSC bots.
> 
> Committed r221876: <http://trac.webkit.org/changeset/221876>

OK, 32bit is failing because MapHash is calculated against 64bit (JSValue bit pattern), while our HashMap uses 32bit pointer hash.

[Yusuke Suzuki]

Created attachment 320554 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:440:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 43 files


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

[Yusuke Suzuki]

Committed r221959: <http://trac.webkit.org/changeset/221959>

[Radar WebKit Bug Importer]

<rdar://problem/34694245>