Bug 197186 – [JSC] Use node index as DFG::MinifiedID

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Use node index as DFG::MinifiedID

[Yusuke Suzuki]

Created attachment 368011 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/50118963>

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:44:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 6 files


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

[Yusuke Suzuki]

Created attachment 368013 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:43:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 9 files


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

[Saam Barati]

Comment on attachment 368013 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGMinifiedID.h:67
> +    uintptr_t bits() const { return m_index; }

What is this used for? Why still return uintptr?

[Yusuke Suzuki]

Comment on attachment 368013 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGMinifiedID.h:67
>> +    uintptr_t bits() const { return m_index; }
> 
> What is this used for? Why still return uintptr?

Nice catch. It is originally introduced to make MinifiedID uintptr_t or some integer types to be included one member of union. But now, C++11 allows us to put this class directly in the union. So, we do not need to have this.
I'll remove it.

[Yusuke Suzuki]

Comment on attachment 368013 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGMinifiedID.h:67
>>> +    uintptr_t bits() const { return m_index; }
>> 
>> What is this used for? Why still return uintptr?
> 
> Nice catch. It is originally introduced to make MinifiedID uintptr_t or some integer types to be included one member of union. But now, C++11 allows us to put this class directly in the union. So, we do not need to have this.
> I'll remove it.

Ah, no. Having non-trivial constructor makes putting it in union difficult. I'll just continue using this bits / fromBits mechanism in this patch.

[Yusuke Suzuki]

Comment on attachment 368013 [details]
Patch

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

>>>> Source/JavaScriptCore/dfg/DFGMinifiedID.h:67
>>>> +    uintptr_t bits() const { return m_index; }
>>> 
>>> What is this used for? Why still return uintptr?
>> 
>> Nice catch. It is originally introduced to make MinifiedID uintptr_t or some integer types to be included one member of union. But now, C++11 allows us to put this class directly in the union. So, we do not need to have this.
>> I'll remove it.
> 
> Ah, no. Having non-trivial constructor makes putting it in union difficult. I'll just continue using this bits / fromBits mechanism in this patch.

And I fixed this, using unsigned instead of uintptr_t.

[Yusuke Suzuki]

Created attachment 368058 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:43:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 10 files


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

[Robin Morisset]

Comment on attachment 368058 [details]
Patch

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

Lgtm overall

> Source/JavaScriptCore/dfg/DFGMinifiedIDInlines.h:35
> +inline MinifiedID::MinifiedID(Node* node)

What is the point of putting this function in its own file?
Considering how trivial it is, I doubt it has any impact on compile time.

[Yusuke Suzuki]

Comment on attachment 368058 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGMinifiedIDInlines.h:35
>> +inline MinifiedID::MinifiedID(Node* node)
> 
> What is the point of putting this function in its own file?
> Considering how trivial it is, I doubt it has any impact on compile time.

This is because we do not put DFGNode.h as Private header (exposed to the rest of the WebKit), while DFGMinifiedID.h is included by ValueRecovery and it is included by some headers that is annotated as Private exposed.
We do not want to make DFGNode.h and dependent headers Private-exposed. To avoid this, we have two choices,

1. Put MinifiedID::MinifiedID implementation in DFGMinifiedID.cpp
2. Add DFGMinifiedIDInlines.h

Here I took (2) option because the implementation of MinifiedID::MinifiedID is small.

[Robin Morisset]

(In reply to Yusuke Suzuki from comment #13)
> Comment on attachment 368058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368058&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGMinifiedIDInlines.h:35
> >> +inline MinifiedID::MinifiedID(Node* node)
> > 
> > What is the point of putting this function in its own file?
> > Considering how trivial it is, I doubt it has any impact on compile time.
> 
> This is because we do not put DFGNode.h as Private header (exposed to the
> rest of the WebKit), while DFGMinifiedID.h is included by ValueRecovery and
> it is included by some headers that is annotated as Private exposed.
> We do not want to make DFGNode.h and dependent headers Private-exposed. To
> avoid this, we have two choices,
> 
> 1. Put MinifiedID::MinifiedID implementation in DFGMinifiedID.cpp
> 2. Add DFGMinifiedIDInlines.h
> 
> Here I took (2) option because the implementation of MinifiedID::MinifiedID
> is small.

That makes sense, thanks for the answer.

[Yusuke Suzuki]

Comment on attachment 368058 [details]
Patch

Thanks Saam and Robin!

[WebKit Commit Bot]

Comment on attachment 368058 [details]
Patch

Rejecting attachment 368058 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 368058, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=368058&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=197186&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 368058 from bug 197186.
Fetching: https://bugs.webkit.org/attachment.cgi?id=368058
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
Can't create session: Unable to connect to a repository at URL 'http://svn.webkit.org/repository/webkit/trunk': Error running context: The server unexpectedly closed the connection. at /Applications/Xcode.app/Contents/Developer/usr/libexec/git-core/git-svn line 1033.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
Can't create session: Unable to connect to a repository at URL 'http://svn.webkit.org/repository/webkit/trunk': Error running context: The server unexpectedly closed the connection. at /Applications/Xcode.app/Contents/Developer/usr/libexec/git-core/git-svn line 1033.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   165bf4efab5..908452fed98  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 244570 = 165bf4efab5cfcf64ee0183401b1003be50e82f5
r244571 = fd4abbc1ad1f0f8c99119c1d366e7c04bf46d69b
r244572 = 908452fed98f80ee058f1b2535fc24e5a6efa3da
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/11976964

[Yusuke Suzuki]

Committed r244578: <https://trac.webkit.org/changeset/244578>