Create and cache method extraction stub in the ICData.

When requested to extract a method M from class C inject a method extractor (consisting of a single AST node CreateClosure) as a getter get:M into C.

This allows to cache and optimize method extraction requests as normal method invocations and at hot method extraction sites that significantly decreases overhead of method extraction which previously required two trips into runtime system and was not cached at all.

BUG=

Committed: https://code.google.com/p/dart/source/detail?r=17261

[Vyacheslav Egorov (Google), 2012-12-18 23:59:21]

This is ready for the review.

[Vyacheslav Egorov (Google), 2013-01-16 14:00:23]

I have removed hand written assembly and specialization in flow graph optimizer
in favor of artificial AST generation.

Please take another look.

[srdjan, 2013-01-16 22:08:56]

LGTM

https://codereview.chromium.org/11642003/diff/2001/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11642003/diff/2001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:938:
String::Handle(Field::NameFromGetter(call->function_name()));
indent 4 spaces

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1130: static const char*
CreateMethodExtractorName(const Function& closure_function) {
ASSERT(closure_function.IsClosureFunction()) ... also below.

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1141: static RawFunction* GetMethodExtractor(const
Function& closure_function) {
Please add a brief comment what a method extractor is for.

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:1383: const bool has_one_target =
unary_checks.HasOneTarget();
Move this down near its use, so it is not unnecessarily invoked.

[Ivan Posva, 2013-01-17 07:24:36]

I still disagree with the LGTM and the approach chosen here.

-Ivan


On 2013/01/16 22:08:56, srdjan wrote:
> LGTM
> 
>
https://codereview.chromium.org/11642003/diff/2001/runtime/vm/flow_graph_opti...
> File runtime/vm/flow_graph_optimizer.cc (right):
> 
>
https://codereview.chromium.org/11642003/diff/2001/runtime/vm/flow_graph_opti...
> runtime/vm/flow_graph_optimizer.cc:938:
> String::Handle(Field::NameFromGetter(call->function_name()));
> indent 4 spaces
> 
>
https://codereview.chromium.org/11642003/diff/6001/runtime/vm/code_generator.cc
> File runtime/vm/code_generator.cc (right):
> 
>
https://codereview.chromium.org/11642003/diff/6001/runtime/vm/code_generator....
> runtime/vm/code_generator.cc:1130: static const char*
> CreateMethodExtractorName(const Function& closure_function) {
> ASSERT(closure_function.IsClosureFunction()) ... also below.
> 
>
https://codereview.chromium.org/11642003/diff/6001/runtime/vm/code_generator....
> runtime/vm/code_generator.cc:1141: static RawFunction*
GetMethodExtractor(const
> Function& closure_function) {
> Please add a brief comment what a method extractor is for.
> 
>
https://codereview.chromium.org/11642003/diff/6001/runtime/vm/flow_graph_opti...
> File runtime/vm/flow_graph_optimizer.cc (right):
> 
>
https://codereview.chromium.org/11642003/diff/6001/runtime/vm/flow_graph_opti...
> runtime/vm/flow_graph_optimizer.cc:1383: const bool has_one_target =
> unary_checks.HasOneTarget();
> Move this down near its use, so it is not unnecessarily invoked.

[Ivan Posva, 2013-01-17 08:03:20]

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:994: // func.token_pos() points to the name of the field.
I don't think this comment is correct. The token_pos does not point to a field.

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/stub_code.cc
File runtime/vm/stub_code.cc (right):

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/stub_code.cc#ne...
runtime/vm/stub_code.cc:11: #include "vm/symbols.h"
Why is this needed?

[Vyacheslav Egorov (Google), 2013-01-17 12:46:38]

I've addressed comments. Please take another look.

Now I am injecting an extractor for the method M into M's owner as a getter
get:M.

I have modified resolution related routines respectively.

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1130: static const char*
CreateMethodExtractorName(const Function& closure_function) {
On 2013/01/16 22:08:56, srdjan wrote:
> ASSERT(closure_function.IsClosureFunction()) ... also below.

This method died.

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1141: static RawFunction* GetMethodExtractor(const
Function& closure_function) {
On 2013/01/16 22:08:56, srdjan wrote:
> Please add a brief comment what a method extractor is for.

Done.

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:1383: const bool has_one_target =
unary_checks.HasOneTarget();
On 2013/01/16 22:08:56, srdjan wrote:
> Move this down near its use, so it is not unnecessarily invoked.

Done.

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:994: // func.token_pos() points to the name of the field.
On 2013/01/17 08:03:20, Ivan Posva wrote:
> I don't think this comment is correct. The token_pos does not point to a
field.

Indeed it is not. token_pos is 0 actually.

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/stub_code.cc
File runtime/vm/stub_code.cc (right):

https://codereview.chromium.org/11642003/diff/6001/runtime/vm/stub_code.cc#ne...
runtime/vm/stub_code.cc:11: #include "vm/symbols.h"
On 2013/01/17 08:03:20, Ivan Posva wrote:
> Why is this needed?

Accidental edit. Fully reverted.

[Vyacheslav Egorov (Google), 2013-01-17 12:47:52]

My main concern here is that lookup function became more heavyweight which might
affect some corner cases.

[Ivan Posva, 2013-01-18 07:00:51]

Resolver and VM class hierarchy related changes LGTM. Code in the optimizer
should be looked at by somebody with more experience in that area.

-Ivan

[Ivan Posva, 2013-01-18 07:00:55]

Resolver and VM class hierarchy related changes LGTM. Code in the optimizer
should be looked at by somebody with more experience in that area.

-Ivan

[Ivan Posva, 2013-01-18 07:00:58]

Resolver and VM class hierarchy related changes LGTM. Code in the optimizer
should be looked at by somebody with more experience in that area.

-Ivan

[Vyacheslav Egorov (Google), 2013-01-18 11:24:40]

Thanks for the review!

Code in the optimizer did not change since Srdjan reviewed it (I just moved one
line of code according to his review comment).

I am going to land this change.