Bug 170210 - LinkBuffer and ExecutableAllocator shouldn't have anything to do with VM
Summary: LinkBuffer and ExecutableAllocator shouldn't have anything to do with VM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks: 168264
  Show dependency treegraph
 
Reported: 2017-03-28 15:51 PDT by Saam Barati
Modified: 2017-03-29 15:56 PDT (History)
12 users (show)

See Also:


Attachments
WIP (37.04 KB, patch)
2017-03-28 19:42 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (115.87 KB, patch)
2017-03-29 13:36 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (117.74 KB, patch)
2017-03-29 13:38 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (121.47 KB, patch)
2017-03-29 14:05 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (134.26 KB, patch)
2017-03-29 14:17 PDT, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (134.31 KB, patch)
2017-03-29 14:33 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-03-28 15:51:40 PDT
...
Comment 1 Saam Barati 2017-03-28 19:42:23 PDT
Created attachment 305695 [details]
WIP
Comment 2 Saam Barati 2017-03-29 13:36:42 PDT
Created attachment 305779 [details]
WIP

lets see what EWS thinks
Comment 3 Saam Barati 2017-03-29 13:38:35 PDT
Created attachment 305780 [details]
WIP

added webcore change
Comment 4 Saam Barati 2017-03-29 14:05:08 PDT
Created attachment 305783 [details]
WIP

fixed some bad lambda capture
Comment 5 Saam Barati 2017-03-29 14:17:14 PDT
Created attachment 305788 [details]
patch
Comment 6 Build Bot 2017-03-29 14:20:25 PDT
Attachment 305788 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Mark Lam 2017-03-29 14:28:13 PDT
Comment on attachment 305788 [details]
patch

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

r=me with suggestions.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5984
> +        VM* vm = &this->vm();

nit: can we use a VM& instead?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6282
> +        VM* vm = &this->vm();

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6408
> +        VM* vm = &this->vm();

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6678
> +        VM* vm = &this->vm();

Ditto.

> Source/JavaScriptCore/jit/ExecutableAllocator.h:141
> +private:
> +

nit: can you put the empty line before the private: section instead of after?

> Source/JavaScriptCore/runtime/VM.cpp:169
>  #if ENABLE(ASSEMBLER)
> -    , executableAllocator(*this)
>  #endif

Remove these lines altogether.
Comment 8 Saam Barati 2017-03-29 14:32:19 PDT
Comment on attachment 305788 [details]
patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5984
>> +        VM* vm = &this->vm();
> 
> nit: can we use a VM& instead?

The reason I did this is that changing to VM& would require 2 more lines of code. And it's also a bit more in line with the style in FTLLower to get a pointer. In principle, I agree, though. We know the thing will never be null.
Comment 9 Saam Barati 2017-03-29 14:33:10 PDT
Created attachment 305794 [details]
patch for landing
Comment 10 WebKit Commit Bot 2017-03-29 15:55:57 PDT
Comment on attachment 305794 [details]
patch for landing

Clearing flags on attachment: 305794

Committed r214571: <http://trac.webkit.org/changeset/214571>
Comment 11 WebKit Commit Bot 2017-03-29 15:56:03 PDT
All reviewed patches have been landed.  Closing bug.