Bug 197186 - [JSC] Use node index as DFG::MinifiedID
Summary: [JSC] Use node index as DFG::MinifiedID
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-22 21:56 PDT by Yusuke Suzuki
Modified: 2019-04-23 18:50 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.87 KB, patch)
2019-04-22 21:59 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.67 KB, patch)
2019-04-22 22:16 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (18.15 KB, patch)
2019-04-23 13:34 PDT, Yusuke Suzuki
sbarati: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-04-22 21:56:57 PDT
[JSC] Use node index as DFG::MinifiedID
Comment 1 Yusuke Suzuki 2019-04-22 21:59:25 PDT
Created attachment 368011 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-04-22 21:59:58 PDT
<rdar://problem/50118963>
Comment 3 Build Bot 2019-04-22 22:01:45 PDT
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.
Comment 4 Yusuke Suzuki 2019-04-22 22:16:12 PDT
Created attachment 368013 [details]
Patch
Comment 5 Build Bot 2019-04-22 22:19:13 PDT
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.
Comment 6 Saam Barati 2019-04-23 08:21:29 PDT
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?
Comment 7 Yusuke Suzuki 2019-04-23 13:08:47 PDT
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.
Comment 8 Yusuke Suzuki 2019-04-23 13:30:16 PDT
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.
Comment 9 Yusuke Suzuki 2019-04-23 13:33:50 PDT
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.
Comment 10 Yusuke Suzuki 2019-04-23 13:34:50 PDT
Created attachment 368058 [details]
Patch
Comment 11 Build Bot 2019-04-23 13:37:56 PDT
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.
Comment 12 Robin Morisset 2019-04-23 13:45:05 PDT
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.
Comment 13 Yusuke Suzuki 2019-04-23 14:42:10 PDT
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.
Comment 14 Robin Morisset 2019-04-23 15:11:47 PDT
(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.
Comment 15 Yusuke Suzuki 2019-04-23 16:48:37 PDT
Comment on attachment 368058 [details]
Patch

Thanks Saam and Robin!
Comment 16 WebKit Commit Bot 2019-04-23 17:16:50 PDT
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 commit-queue@webkit.org...
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
Comment 17 Yusuke Suzuki 2019-04-23 18:50:54 PDT
Committed r244578: <https://trac.webkit.org/changeset/244578>