VM: Zero-cost try-finally with an empty finally-block.

VM: Zero-cost try-finally with an empty finally-block.

Consider code like

try {
    something();
} finally {
    assert(expr);
}

The compiler de-sugars this into

try {
  something();
} catch (_) {
  rethrow;
} finally {
  assert(expr);
}

In unchecked mode this leaves an empty finally-block, but we would still
have the overhead of the try-catch. This CL avoids this unnecessary overhead
by making try-finally with an empty finally zero-cost.

BUG=#27274
R=rmacnak@google.com

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

[Florian Schneider, 2016-09-08 21:46:14]

fschneider@google.com changed reviewers:
+ rmacnak@google.com

[rmacnak, 2016-09-08 22:02:38]

https://codereview.chromium.org/2322853003/diff/1/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/2322853003/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:4230: if ((finally_block != NULL) &&
(finally_block->length() == 0)) {
finally_block == NULL || finally_block->length() == 0 ?

try {
  ...
} catch (e) {
  rethrow;
}

should be okay too.

https://codereview.chromium.org/2322853003/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:4233: ThrowNode* throw_node = catch_sequence->
NodeAt(0)->AsThrowNode();
->NodeAt

[Florian Schneider, 2016-09-09 00:51:48]

https://codereview.chromium.org/2322853003/diff/1/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/2322853003/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:4230: if ((finally_block != NULL) &&
(finally_block->length() == 0)) {
On 2016/09/08 22:02:37, rmacnak wrote:
> finally_block == NULL || finally_block->length() == 0 ?
> 
> try {
>   ...
> } catch (e) {
>   rethrow;
> }
> 
> should be okay too.

Yes, the only issue is that when 'catch' is parsed, it translated into:

-Catch
 -Seqence
  -Sequence
   -StoreLocal(e)  // if exception var present
   -StoreLocal(s)  // if stacktrace var present
   -Sequence
    -Rethrow

With an 'on' handler there is also an is-test.
This all makes the pattern matching more complicated and brittle depending on
many details in the parser implementation. I'll stick to the simpler synthetic
catch clauses for now.

https://codereview.chromium.org/2322853003/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:4233: ThrowNode* throw_node = catch_sequence->
NodeAt(0)->AsThrowNode();
On 2016/09/08 22:02:37, rmacnak wrote:
> ->NodeAt

Done.

https://codereview.chromium.org/2322853003/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:4234: if ((throw_node != NULL) &&
(throw_node->exception() != NULL)) {
Actually, I have to check (throw_node->stack_trace() != NULL) to detect the
rethrow correctly. It worked because catch-clauses parsed from Dart code are
parsed differently.

[rmacnak, 2016-09-09 20:24:35]

lgtm

https://chromiumcodereview.appspot.com/2322853003/diff/20001/runtime/vm/flow_...
File runtime/vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/2322853003/diff/20001/runtime/vm/flow_...
runtime/vm/flow_graph_builder.cc:4235: ThrowNode* throw_node = catch_sequence->
NodeAt(0)->AsThrowNode();
extra space in "-> NodeAt"

[Florian Schneider, 2016-09-09 20:30:07]

https://chromiumcodereview.appspot.com/2322853003/diff/20001/runtime/vm/flow_...
File runtime/vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/2322853003/diff/20001/runtime/vm/flow_...
runtime/vm/flow_graph_builder.cc:4235: ThrowNode* throw_node = catch_sequence->
NodeAt(0)->AsThrowNode();
On 2016/09/09 20:24:35, rmacnak wrote:
> extra space in "-> NodeAt"

Done.

[Florian Schneider, 2016-09-09 21:00:55]

Description was changed from

==========
VM: Zero-cost try-finally with an empty finally-block.

Consider code like

try {
    something();
} finally {
    assert(expr);
}

The compiler de-sugars this into

try {
  something();
} catch (_) {
  rethrow;
} finally {
  assert(expr);
}

In unchecked mode this leaves an empty finally-block, but we would still
have the overhead of the try-catch. This CL avoids this unnecessary overhead
by making try-finally with an empty finally zero-cost.

BUG=#27274
==========

to

==========
VM: Zero-cost try-finally with an empty finally-block.

Consider code like

try {
    something();
} finally {
    assert(expr);
}

The compiler de-sugars this into

try {
  something();
} catch (_) {
  rethrow;
} finally {
  assert(expr);
}

In unchecked mode this leaves an empty finally-block, but we would still
have the overhead of the try-catch. This CL avoids this unnecessary overhead
by making try-finally with an empty finally zero-cost.

BUG=#27274
R=rmacnak@google.com

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

[Florian Schneider, 2016-09-09 21:00:57]

Committed patchset #4 (id:60001) manually as
60e6faa7c3357ab519fc88b3f51f39b5419eb257 (presubmit successful).