search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.09k stars 1.56k forks source link

[Dart2JS] Inconsistent side effects in parameter lists #26243

Closed lexaknyazev closed 8 years ago

lexaknyazev commented 8 years ago

Looks like try-catch block affects order of operations in parameter lists. For example: https://dartpad.dartlang.org/1bfecc33644e9f722fdc60425a499c62

Dart2JS:

foo 0 1
bar 1 1
baz-notry 1 1
baz-try 1 1

DartVM:

foo 0 1
bar 0 1
baz-notry 0 1
baz-try 0 1
fsc8000 commented 8 years ago

@rakudrama

kmillikin commented 8 years ago

For some reason it looks like HLocalGet and HLocalSet do not have any side effects so they are pure, so the SsaInstructionMerger thinks it can move them. In this case for bar before instruction merging:

B3
v7  LocalGet : v2.i [null|subclass=Object]    
v8  LocalGet : v2.i [null|subclass=Object]    
U71 OneShotInterceptor : u13.+(v8, i9)([null|subclass=Object]) Union of [[exact=JSString], [subclass=JSNumber]] ! ? 
c12 LocalSet : v2.i to U71 [empty]    
s23 Stringify : v7 [exact=JSString] ! ? 
s24 StringConcat : s69 + s23 [exact=JSString]   ? 
s26 StringConcat : s24 + s19 [exact=JSString]   ? 
s28 Stringify : U71 [exact=JSString] ! ? 
s29 StringConcat : s26 + s28 [exact=JSString]   ? 
c38 Goto : (B4) [empty]

And after:

B3
v8  LocalGet : v2.i [null|subclass=Object]    
U71 OneShotInterceptor : u13.+(v8, i9)([null|subclass=Object]) Union of [[exact=JSString], [subclass=JSNumber]] ! ? 
c12 LocalSet : v2.i to U71 [empty]    
v7  LocalGet : v2.i [null|subclass=Object]    
s23 Stringify : v7 [exact=JSString] ! ? 
s24 StringConcat : s69 + s23 [exact=JSString]   ? 
s26 StringConcat : s24 + s19 [exact=JSString]   ? 
s28 Stringify : U71 [exact=JSString] ! ? 
s29 StringConcat : s26 + s28 [exact=JSString]   ? 
c38 Goto : (B4) [empty]

You can see that v7 got moved below c12.

lexaknyazev commented 8 years ago

Is there any chance for this issue to be fixed in 1.16?

fsc8000 commented 8 years ago

@mit-mit @johnniwinther

rakudrama commented 8 years ago

Fix available: https://codereview.chromium.org/1914623002

mit-mit commented 8 years ago

@whesse I guess we should merge this even though there is no merge request yet?

mit-mit commented 8 years ago

We won't be able to get this in today before we snap the 1.16 stable build, so the fix will go into the next 1.17 dev build.

rakudrama commented 8 years ago

I have found a problem with the fix, so I will revert it and get it right. Sorry, Alexey, it must be 1.17.

You can work around the bug by lifting code into closures: https://dartpad.dartlang.org/165c53f9de621e249ede0f32fcdb9aca

It is a good idea to put as little code as possible in a function that contains try-catch since most JavaScript engines don't bother to optimize such functions.

On Mon, Apr 25, 2016 at 4:25 AM, Michael Thomsen notifications@github.com wrote:

We won't be able to get this in today before we snap the 1.16 stable build, so the fix will go into the nex 1.17 dev build.

— You are receiving this because you were assigned. Reply to this email directly or view it on GitHub https://github.com/dart-lang/sdk/issues/26243#issuecomment-214272660

rakudrama commented 8 years ago

The revised fix was committed.