Improve the performance of closure-converted code.

Improve the performance of closure-converted code.

Summary:

Previously, we would create a wrapper function in the flowgraph around converted
closures, which would forward all the closure's arguments and unpack the context
argument before calling the real closure function.

Now, we perform the unpacking at the top of the real function to avoid having
any wrapper function.

Previously, captured parameters would still be appear live to the GC even if
they're updated, because after they are copied into the context, all updates to
them are done there.

Now, as in regular closures, we zero-out the parameter variable after copying
it's value into the context, avoiding potential memory leaks.

Test Plan:

Ran the closure conversion test suite.
Ran benchmarks on Golem -- all statistically significant regressions are gone.

BUG=
R=dmitryas@google.com, regis@google.com

Committed: https://github.com/dart-lang/sdk/commit/f0941f7c7d5c03fadf299c9f41962b98a8ed1db7

[sjindel, 2017-08-30 16:18:55]

sjindel@google.com changed reviewers:
+ dmitryas@google.com, regis@google.com

[sjindel, 2017-08-30 16:26:40]

Description was changed from

==========
Improve the performance of closure-converted code.

Summary:

Previously, we would create a wrapper function in the flowgraph around converted
closures, which would forward all the closure's arguments and unpack the context
argument before calling the real closure function.

Now, we perform the unpacking at the top of the real function to avoid having
any wrapper function.

Previously, captured parameters would still be appear live to the GC even if
they're updated, because after they are copied into the context, all updates to
them are done there.

Now, as in regular closures, we zero-out the parameter variable after copying
it's value into the context, avoiding potential memory leaks.

Test Plan:

Ran the closure conversion test suite.
Ran benchmarks on Golem:

before:
http://146.148.43.81:8080/Comparison#targetA%3Ddart-dfe%3BmachineTypeA%3Dlinu...

after:
http://146.148.43.81:8080/Comparison#targetA%3Ddart-dfe%3BmachineTypeA%3Dlinu...

The failed benchmarks are spurious failures.

BUG=
==========

to

==========
Improve the performance of closure-converted code.

Summary:

Previously, we would create a wrapper function in the flowgraph around converted
closures, which would forward all the closure's arguments and unpack the context
argument before calling the real closure function.

Now, we perform the unpacking at the top of the real function to avoid having
any wrapper function.

Previously, captured parameters would still be appear live to the GC even if
they're updated, because after they are copied into the context, all updates to
them are done there.

Now, as in regular closures, we zero-out the parameter variable after copying
it's value into the context, avoiding potential memory leaks.

Test Plan:

Ran the closure conversion test suite.
Ran benchmarks on Golem -- all statistically significant regressions are gone.

BUG=
==========

[regis, 2017-08-30 16:28:19]

The VM changes LGTM.
Thanks

[Dmitry Stefantsov, 2017-09-01 12:42:30]

This great optimization LGTM! :)

The remark bellow is a small nitpick about phrasing in a comment.

https://codereview.chromium.org/3008923002/diff/1/pkg/kernel/lib/transformati...
File pkg/kernel/lib/transformations/closure/rewriter.dart (right):

https://codereview.chromium.org/3008923002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/closure/rewriter.dart:30: /// Inserts an
expression that sets a parameter to NULL, so we don't have
I believe some text is lost between "have" and "unnecessary" :)

[sjindel, 2017-09-01 12:59:17]

https://codereview.chromium.org/3008923002/diff/1/pkg/kernel/lib/transformati...
File pkg/kernel/lib/transformations/closure/rewriter.dart (right):

https://codereview.chromium.org/3008923002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/closure/rewriter.dart:30: /// Inserts an
expression that sets a parameter to NULL, so we don't have
On 2017/09/01 12:42:29, Dmitry Stefantsov wrote:
> I believe some text is lost between "have" and "unnecessary" :)

Acknowledged.

[sjindel, 2017-09-01 12:59:45]

Description was changed from

==========
Improve the performance of closure-converted code.

Summary:

Previously, we would create a wrapper function in the flowgraph around converted
closures, which would forward all the closure's arguments and unpack the context
argument before calling the real closure function.

Now, we perform the unpacking at the top of the real function to avoid having
any wrapper function.

Previously, captured parameters would still be appear live to the GC even if
they're updated, because after they are copied into the context, all updates to
them are done there.

Now, as in regular closures, we zero-out the parameter variable after copying
it's value into the context, avoiding potential memory leaks.

Test Plan:

Ran the closure conversion test suite.
Ran benchmarks on Golem -- all statistically significant regressions are gone.

BUG=
==========

to

==========
Improve the performance of closure-converted code.

Summary:

Previously, we would create a wrapper function in the flowgraph around converted
closures, which would forward all the closure's arguments and unpack the context
argument before calling the real closure function.

Now, we perform the unpacking at the top of the real function to avoid having
any wrapper function.

Previously, captured parameters would still be appear live to the GC even if
they're updated, because after they are copied into the context, all updates to
them are done there.

Now, as in regular closures, we zero-out the parameter variable after copying
it's value into the context, avoiding potential memory leaks.

Test Plan:

Ran the closure conversion test suite.
Ran benchmarks on Golem -- all statistically significant regressions are gone.

BUG=
R=dmitryas@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/f0941f7c7d5c03fadf299c9f41962b98a8ed1db7
==========

[sjindel, 2017-09-01 12:59:47]

Committed patchset #2 (id:20001) manually as
f0941f7c7d5c03fadf299c9f41962b98a8ed1db7 (presubmit successful).