Bug 147508 - jsc-tailcall: We should reuse the frame efficiently in the DFG instead of doing a memmove
Summary: jsc-tailcall: We should reuse the frame efficiently in the DFG instead of doi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basile Clement
URL:
Keywords:
Depends on:
Blocks: 146477
  Show dependency treegraph
 
Reported: 2015-07-31 14:52 PDT by Basile Clement
Modified: 2015-08-25 15:59 PDT (History)
1 user (show)

See Also:


Attachments
Patch (164.17 KB, patch)
2015-08-21 14:52 PDT, Basile Clement
no flags Details | Formatted Diff | Diff
Patch (130.84 KB, patch)
2015-08-21 15:53 PDT, Basile Clement
no flags Details | Formatted Diff | Diff
Patch (131.28 KB, patch)
2015-08-24 19:17 PDT, Basile Clement
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basile Clement 2015-07-31 14:52:59 PDT
...
Comment 1 Basile Clement 2015-08-21 14:52:41 PDT
Created attachment 259665 [details]
Patch
Comment 2 Basile Clement 2015-08-21 15:53:53 PDT
Created attachment 259677 [details]
Patch
Comment 3 Michael Saboff 2015-08-23 21:32:04 PDT
Comment on attachment 259677 [details]
Patch

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

Add blank lines between function declarations or definitions.
Let's use = initialization for "for" loops.

r=me with some comment clean up and stylistic changes.

> Source/JavaScriptCore/jit/CallFrameShuffleData.cpp:43
> +    for (size_t i { 0 }; i < registerSaveLocations->size(); ++i) {

Let's use = initialization for "for" loops.

> Source/JavaScriptCore/jit/CallFrameShuffleData.cpp:55
> +    for (Reg reg { Reg::first() }; reg <= Reg::last(); reg = reg.next()) {

Let's use = initialization for "for" loops.

> Source/JavaScriptCore/jit/CallFrameShuffler.h:41
> +// A CachedRecovery is a simple wrapper around a ValueRecovery that 

That what?  Complete the comment.

> Source/JavaScriptCore/jit/CallFrameShuffler.h:106
> +

Delete one blank line.

> Source/JavaScriptCore/jit/CallFrameShuffler.h:451
> +    int m_frameDelta { m_alignedNewFrameSize - m_alignedOldFrameSize };

Initialize this in the constructor.

> Source/JavaScriptCore/jit/CallFrameShuffler.h:685
> +    //    pointer, and m_oldFrameOffset is 0. We could 

We could what?  Finish the comment.
Comment 4 Basile Clement 2015-08-24 19:17:14 PDT
Comment on attachment 259677 [details]
Patch

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

Addressed the comments below + added blank lines between method definitions.

>> Source/JavaScriptCore/jit/CallFrameShuffleData.cpp:43
>> +    for (size_t i { 0 }; i < registerSaveLocations->size(); ++i) {
> 
> Let's use = initialization for "for" loops.

Changed all of those.

>> Source/JavaScriptCore/jit/CallFrameShuffler.h:41
>> +// A CachedRecovery is a simple wrapper around a ValueRecovery that 
> 
> That what?  Complete the comment.

Oh, oops. I should actually reword it since it's a bit more than a "simple wrapper". Changed to "A CachedRecovery is a wrapper around a ValueRecovery that records where said value should go on the stack and/or in registers. Whenever we perform an operation changing the ValueRecovery, we update the CachedRecovery's member in place."

>> Source/JavaScriptCore/jit/CallFrameShuffler.h:106
>> +
> 
> Delete one blank line.

Done.

>> Source/JavaScriptCore/jit/CallFrameShuffler.h:451
>> +    int m_frameDelta { m_alignedNewFrameSize - m_alignedOldFrameSize };
> 
> Initialize this in the constructor.

I did this here because I believed it to make it clearer what happens when reading the comment. Moved to the constructor.

>> Source/JavaScriptCore/jit/CallFrameShuffler.h:685
>> +    //    pointer, and m_oldFrameOffset is 0. We could 
> 
> We could what?  Finish the comment.

I'll just remove the "We could" part. I wanted to say that we could use the stack pointer as well, but that'd just be noise.
Comment 5 Basile Clement 2015-08-24 19:17:20 PDT
Created attachment 259806 [details]
Patch
Comment 6 Michael Saboff 2015-08-25 15:24:11 PDT
Comment on attachment 259806 [details]
Patch

r=me
Comment 7 Basile Clement 2015-08-25 15:59:58 PDT
Committed r188937 <http://trac.webkit.org/changeset/188937>