Flushing of code from functions that we expect not to use again....

Flushing of code from functions that we expect not to use again.

This adds an additional step to full gc, removing code from functions
that are no longer in the compilation cache. The code is replaced with
a lazy compile version enabling us to recompile the function in case
we do actually need it again.


Committed: http://code.google.com/p/v8/source/detail?r=4814

[Rico, 2010-06-04 12:32:56]

Mads and Slava,
Could you have a look.

[Mads Ager (chromium), 2010-06-07 08:31:11]

http://codereview.chromium.org/2632003/diff/9001/10001
File src/compilation-cache.cc (right):

http://codereview.chromium.org/2632003/diff/9001/10001#newcode219
src/compilation-cache.cc:219: CompilationCacheTable::cast(tables_[generation]);
Four-space indent.

http://codereview.chromium.org/2632003/diff/9001/10001#newcode221
src/compilation-cache.cc:221: Object* object =
table->Lookup(String::cast(script->source()));
Extract the source before the loop?

http://codereview.chromium.org/2632003/diff/9001/10002
File src/compilation-cache.h (right):

http://codereview.chromium.org/2632003/diff/9001/10002#newcode83
src/compilation-cache.h:83: static bool HasFunction(SharedFunctionInfo* sfi);
I like function_info better than sfi.

http://codereview.chromium.org/2632003/diff/9001/10004
File src/heap.cc (right):

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2190
src/heap.cc:2190: // the code contains the stack pointer from any of the stacks.
It is the other way around:

to see if there are activations on any of the stack corresponding to the code.

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2211
src/heap.cc:2211: void FlushFunction(SharedFunctionInfo* sfi) {
FlushFunction -> FlushCodeForFunction?

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2216
src/heap.cc:2216: // We never delete Api functions.
delete -> flush code for

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2219
src/heap.cc:2219: // This must be a function type.
Only flush code for functions.

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2225
src/heap.cc:2225: // If this function is in the compilation cache we don't
remove it.
don't remove it -> do not flush the code.

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2245
src/heap.cc:2245: // Do not flush code if debug is loaded.
debug is loaded -> the debugger is loaded?

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2251
src/heap.cc:2251: // Don't remove unnamed functions.
I don't think I understand why we cannot flush the code for anonymous functions?
Could you explain why or allow it? :)

http://codereview.chromium.org/2632003/diff/9001/10005
File src/heap.h (right):

http://codereview.chromium.org/2632003/diff/9001/10005#newcode1269
src/heap.h:1269: static void FlushCode();
Please add a comment.

[Mads Ager (chromium), 2010-06-07 08:31:45]

Oh, and this looks great once the comments are addressed! :)

On 2010/06/07 08:31:11, Mads Ager wrote:
> http://codereview.chromium.org/2632003/diff/9001/10001
> File src/compilation-cache.cc (right):
> 
> http://codereview.chromium.org/2632003/diff/9001/10001#newcode219
> src/compilation-cache.cc:219:
CompilationCacheTable::cast(tables_[generation]);
> Four-space indent.
> 
> http://codereview.chromium.org/2632003/diff/9001/10001#newcode221
> src/compilation-cache.cc:221: Object* object =
> table->Lookup(String::cast(script->source()));
> Extract the source before the loop?
> 
> http://codereview.chromium.org/2632003/diff/9001/10002
> File src/compilation-cache.h (right):
> 
> http://codereview.chromium.org/2632003/diff/9001/10002#newcode83
> src/compilation-cache.h:83: static bool HasFunction(SharedFunctionInfo* sfi);
> I like function_info better than sfi.
> 
> http://codereview.chromium.org/2632003/diff/9001/10004
> File src/heap.cc (right):
> 
> http://codereview.chromium.org/2632003/diff/9001/10004#newcode2190
> src/heap.cc:2190: // the code contains the stack pointer from any of the
stacks.
> It is the other way around:
> 
> to see if there are activations on any of the stack corresponding to the code.
> 
> http://codereview.chromium.org/2632003/diff/9001/10004#newcode2211
> src/heap.cc:2211: void FlushFunction(SharedFunctionInfo* sfi) {
> FlushFunction -> FlushCodeForFunction?
> 
> http://codereview.chromium.org/2632003/diff/9001/10004#newcode2216
> src/heap.cc:2216: // We never delete Api functions.
> delete -> flush code for
> 
> http://codereview.chromium.org/2632003/diff/9001/10004#newcode2219
> src/heap.cc:2219: // This must be a function type.
> Only flush code for functions.
> 
> http://codereview.chromium.org/2632003/diff/9001/10004#newcode2225
> src/heap.cc:2225: // If this function is in the compilation cache we don't
> remove it.
> don't remove it -> do not flush the code.
> 
> http://codereview.chromium.org/2632003/diff/9001/10004#newcode2245
> src/heap.cc:2245: // Do not flush code if debug is loaded.
> debug is loaded -> the debugger is loaded?
> 
> http://codereview.chromium.org/2632003/diff/9001/10004#newcode2251
> src/heap.cc:2251: // Don't remove unnamed functions.
> I don't think I understand why we cannot flush the code for anonymous
functions?
> Could you explain why or allow it? :)
> 
> http://codereview.chromium.org/2632003/diff/9001/10005
> File src/heap.h (right):
> 
> http://codereview.chromium.org/2632003/diff/9001/10005#newcode1269
> src/heap.h:1269: static void FlushCode();
> Please add a comment.

[Rico, 2010-06-07 12:16:58]

All comments addressed.

http://codereview.chromium.org/2632003/diff/9001/10001
File src/compilation-cache.cc (right):

http://codereview.chromium.org/2632003/diff/9001/10001#newcode219
src/compilation-cache.cc:219: CompilationCacheTable::cast(tables_[generation]);
On 2010/06/07 08:31:11, Mads Ager wrote:
> Four-space indent.

Done.

http://codereview.chromium.org/2632003/diff/9001/10001#newcode221
src/compilation-cache.cc:221: Object* object =
table->Lookup(String::cast(script->source()));
On 2010/06/07 08:31:11, Mads Ager wrote:
> Extract the source before the loop?

Done.

http://codereview.chromium.org/2632003/diff/9001/10002
File src/compilation-cache.h (right):

http://codereview.chromium.org/2632003/diff/9001/10002#newcode83
src/compilation-cache.h:83: static bool HasFunction(SharedFunctionInfo* sfi);
On 2010/06/07 08:31:11, Mads Ager wrote:
> I like function_info better than sfi.
Changed everywhere

http://codereview.chromium.org/2632003/diff/9001/10004
File src/heap.cc (right):

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2190
src/heap.cc:2190: // the code contains the stack pointer from any of the stacks.
On 2010/06/07 08:31:11, Mads Ager wrote:
> It is the other way around:
> 
> to see if there are activations on any of the stack corresponding to the code.

Done.

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2211
src/heap.cc:2211: void FlushFunction(SharedFunctionInfo* sfi) {
On 2010/06/07 08:31:11, Mads Ager wrote:
> FlushFunction -> FlushCodeForFunction?

Done.

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2216
src/heap.cc:2216: // We never delete Api functions.
On 2010/06/07 08:31:11, Mads Ager wrote:
> delete -> flush code for

Done.

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2219
src/heap.cc:2219: // This must be a function type.
On 2010/06/07 08:31:11, Mads Ager wrote:
> Only flush code for functions.

Done.

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2225
src/heap.cc:2225: // If this function is in the compilation cache we don't
remove it.
On 2010/06/07 08:31:11, Mads Ager wrote:
> don't remove it -> do not flush the code.

Done.

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2245
src/heap.cc:2245: // Do not flush code if debug is loaded.
On 2010/06/07 08:31:11, Mads Ager wrote:
> debug is loaded -> the debugger is loaded?

Done.

http://codereview.chromium.org/2632003/diff/9001/10004#newcode2251
src/heap.cc:2251: // Don't remove unnamed functions.
On 2010/06/07 08:31:11, Mads Ager wrote:
> I don't think I understand why we cannot flush the code for anonymous
functions?
> Could you explain why or allow it? :)
As discussed offline this caught the cases where a complete script is
encapsulated in a function if compiled using Script::Compile. This has been
removed and a check for is_toplevel() is included in the FlushFunction method.
This allows us to also flush code for anonymous functions.

http://codereview.chromium.org/2632003/diff/9001/10005
File src/heap.h (right):

http://codereview.chromium.org/2632003/diff/9001/10005#newcode1269
src/heap.h:1269: static void FlushCode();
On 2010/06/07 08:31:11, Mads Ager wrote:
> Please add a comment.

Done.

[Mads Ager (chromium), 2010-06-07 12:25:36]

LGTM

http://codereview.chromium.org/2632003/diff/24001/25004
File src/heap.cc (right):

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2225
src/heap.cc:2225: //
Complete comment?

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2228
src/heap.cc:2228: // If this function is in the compilation cache we flush the
code.
we flush -> we do not flush

[Vitaly Repeshko, 2010-06-07 12:36:56]

LGTM with a few nits.

http://codereview.chromium.org/2632003/diff/24001/25004
File src/heap.cc (right):

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2196
src/heap.cc:2196: for (StackFrameIterator it(top); !it.done(); it.Advance()) {
We can bail out early if found_ is already true.

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2211
src/heap.cc:2211: void FlushCodeForFunction(SharedFunctionInfo* function_info) {
nit: Missing "static".

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2220
src/heap.cc:2220: if (!function_info->code()->kind() == Code::FUNCTION) return;
Does this prevent repeated flushing of a single shared function info that is
reachable via several closures?

http://codereview.chromium.org/2632003/diff/24001/25007
File src/objects.h (right):

http://codereview.chromium.org/2632003/diff/24001/25007#newcode3309
src/objects.h:3309: inline bool allows_lazy_compilation();
This needs a comment.

[Rico, 2010-06-07 13:28:44]

Thanks for the additional comments Vitaly.
I have addressed them and in addition I have added a small test case to
test-heap.cc

http://codereview.chromium.org/2632003/diff/24001/25004
File src/heap.cc (right):

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2196
src/heap.cc:2196: for (StackFrameIterator it(top); !it.done(); it.Advance()) {
On 2010/06/07 12:36:56, Vitaly wrote:
> We can bail out early if found_ is already true.

Done.

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2211
src/heap.cc:2211: void FlushCodeForFunction(SharedFunctionInfo* function_info) {
On 2010/06/07 12:36:56, Vitaly wrote:
> nit: Missing "static".

Done.

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2220
src/heap.cc:2220: if (!function_info->code()->kind() == Code::FUNCTION) return;
On 2010/06/07 12:36:56, Vitaly wrote:
> Does this prevent repeated flushing of a single shared function info that is
> reachable via several closures?
This is actually already prevented by the is_compiled check earlier on (this
will return false when we have flushed the code).

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2225
src/heap.cc:2225: //
On 2010/06/07 12:25:36, Mads Ager wrote:
> Complete comment?

Done.

http://codereview.chromium.org/2632003/diff/24001/25004#newcode2228
src/heap.cc:2228: // If this function is in the compilation cache we flush the
code.
On 2010/06/07 12:25:36, Mads Ager wrote:
> we flush -> we do not flush

Done.

http://codereview.chromium.org/2632003/diff/24001/25007
File src/objects.h (right):

http://codereview.chromium.org/2632003/diff/24001/25007#newcode3309
src/objects.h:3309: inline bool allows_lazy_compilation();
On 2010/06/07 12:36:56, Vitaly wrote:
> This needs a comment.

Done.

[Mads Ager (chromium), 2010-06-07 14:12:42]

Still LGTM