Issue 2804113002: DDC fix for foreign futures

Issue 2804113002: DDC fix for foreign futures (Closed)

Created:
3 years, 8 months ago by vsm

Modified:
3 years, 8 months ago

Reviewers:
Jennifer Messerly

CC:
dev-compiler+reviews_dartlang.org

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

DDC fix for foreign futures Fixes #29287 R=jmesserly@google.com Committed: https://github.com/dart-lang/sdk/commit/5c3bcff375bae0f2ca383aca49f71a3b63c469e2

Patch Set 2 : Optimize #

Created: 3 years, 8 months ago

(Patch set is too large to download)

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+122 lines, -14 lines)			Patch
M	pkg/dev_compiler/lib/sdk/ddc_sdk.sum	View	1	Binary file		0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/BenchmarkBase.js	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/async_helper.js	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/closure.js	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/destructuring.js	View	1	1 chunk	+4 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/es6_modules.js	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/map_keys.js	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/node_modules.js	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/script.js	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/sunflower/sunflower.js	View	1	1 chunk	+5 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/test/codegen_expected/varargs.js	View	1	1 chunk	+4 lines, -1 line	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart	View	1	3 chunks	+16 lines, -4 lines	0 comments	Download
A	tests/language_strong/async_await_foreign_test.dart	View	1	1 chunk	+72 lines, -0 lines	0 comments	Download

Messages

Total messages: 8 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

vsm

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart (right): https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart#newcode30 pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:30: let _Future = $Future.new(() => null).runtimeType; Is there a ...

3 years, 8 months ago (2017-04-06 19:45:05 UTC) #3

Jennifer Messerly

couple of suggestions but lgtm https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart (right): https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart#newcode29 pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:29: let FutureT = ${getGenericClass(Future)}($T); ...

3 years, 8 months ago (2017-04-06 20:01:52 UTC) #4

couple of suggestions but lgtm

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart
(right):

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:29: let
FutureT = ${getGenericClass(Future)}($T);
const FutureT = ... ?

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:30: let
_Future = $Future.new(() => null).runtimeType;
On 2017/04/06 19:45:05, vsm wrote:
> Is there a better way to get this type?

good question. I'd do it like this:

    let _FutureType;

... more notes below.

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:52: if
(!$instanceOf(future, _Future)) {
you could just match the exact runtime type here, rather than an "is" check?

    if ($_getRuntimeType(future) !== _FutureType) {

... should be a bit faster.

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:58: return
FutureT.microtask(function() {
    let result = FutureT.microtask(function() { ... });
    // TODO(jmesserly): optimize this further.
    _FutureType = ${_getRuntimeType(getGenericClass(result)}();
    return result;

Basically, it's similar to your approach but avoids allocating another Future.
We can get the runtime type from the one we already have by hopping to
_Future<T> and then applying <dynamic>.

... we really should fix this though. We need a way to get the library objects
reliably.

A simpler alternative is:

    const _FutureType = async._Future;

and just rely on our naming scheme? Seems okay to me but I'm not sure if it
works.

vsm

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart (right): https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart#newcode29 pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:29: let FutureT = ${getGenericClass(Future)}($T); On 2017/04/06 20:01:52, Jennifer Messerly ...

3 years, 8 months ago (2017-04-06 21:31:32 UTC) #5

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart
(right):

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:29: let
FutureT = ${getGenericClass(Future)}($T);
On 2017/04/06 20:01:52, Jennifer Messerly wrote:
> const FutureT = ... ?

Done.

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:30: let
_Future = $Future.new(() => null).runtimeType;
On 2017/04/06 20:01:52, Jennifer Messerly wrote:
> On 2017/04/06 19:45:05, vsm wrote:
> > Is there a better way to get this type?
> 
> good question. I'd do it like this:
> 
>     let _FutureType;
> 
> ... more notes below.

Acknowledged.

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:52: if
(!$instanceOf(future, _Future)) {
On 2017/04/06 20:01:52, Jennifer Messerly wrote:
> you could just match the exact runtime type here, rather than an "is" check?
> 
>     if ($_getRuntimeType(future) !== _FutureType) {
> 
> ... should be a bit faster.

I don't think that's quite right - _Future<int> != _Future<dynamic> but doesn't
need to be wrapped.
But, modified to work with getGenericClass - that works.

https://codereview.chromium.org/2804113002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/generators.dart:58: return
FutureT.microtask(function() {
On 2017/04/06 20:01:52, Jennifer Messerly wrote:
>     let result = FutureT.microtask(function() { ... });
>     // TODO(jmesserly): optimize this further.
>     _FutureType = ${_getRuntimeType(getGenericClass(result)}();
>     return result;
> 
> Basically, it's similar to your approach but avoids allocating another Future.
> We can get the runtime type from the one we already have by hopping to
> _Future<T> and then applying <dynamic>.
> 
> ... we really should fix this though. We need a way to get the library objects
> reliably.
> 
> A simpler alternative is:
> 
>     const _FutureType = async._Future;
> 
> and just rely on our naming scheme? Seems okay to me but I'm not sure if it
> works.

Clever!  Done with some slight mods.

vsm

3 years, 8 months ago (2017-04-06 21:53:14 UTC) #8

Message was sent while issue was closed.

Committed patchset #2 (id:10001) manually as
5c3bcff375bae0f2ca383aca49f71a3b63c469e2 (presubmit successful).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages