Skip to content

Remove source mapping from async methods #44374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nshahan opened this issue Dec 2, 2020 · 9 comments
Open

Remove source mapping from async methods #44374

nshahan opened this issue Dec 2, 2020 · 9 comments
Assignees
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. P2 A bug or feature request we're likely to work on web-dev-compiler web-triage-1 priority assigned

Comments

@nshahan
Copy link
Contributor

nshahan commented Dec 2, 2020

If we remove the source map information from the outer function of the generated code below it would potentially avoid showing the redundant stack frames when debugging with Dart DevTools.

Dart:

foo() async => bar();

Generated JavaScript:

  module.foo = function foo() { // (1)
    return async.async(dart.dynamic, function* foo() { // (2)
      return yield bar.bar();
    });
  };

We need to be careful that we don't drastically regress debugging in chrome devtools in the process.

@nshahan nshahan added web-dev-compiler area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. P1 A high priority bug; for example, a single project is unusable or has many test failures labels Dec 2, 2020
@franklinyow franklinyow added this to the January 2021 milestone Jan 5, 2021
@vsmenon
Copy link
Member

vsmenon commented Jan 7, 2021

@nshahan @sigmundch - do we have an owner on this? It's currently marked Jan.

@nshahan
Copy link
Contributor Author

nshahan commented Jan 8, 2021

@mariamhas @grouma Can you help us gauge the priority of this issue? Should this be considered a blocker for the next release?

@grouma
Copy link
Member

grouma commented Jan 8, 2021

I don't see this as being a blocker but it's certainly a nice to have fix.

@franklinyow
Copy link
Contributor

@nshahan I'm assigning you as the owner for now. Feel free to reassign if needed.

@sigmundch sigmundch added P2 A bug or feature request we're likely to work on and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Jan 11, 2021
@nshahan
Copy link
Contributor Author

nshahan commented Jan 13, 2021

After investigating I found the situation is a little different than described. Given:

  module.foo = function foo() { // (1)
    return async.async(dart.dynamic, function* foo() { // (2)
      return yield bar.bar(); // (3)
    });
  };

There are actually no source mapped locations for line (1) so nothing to remove there.

When paused at the line (3) two stack frames appear labeled "foo". The frames correspond to the source mapped locations on lines (2) and (3).

With the current implementation in webdev, I can't find a way to avoid two stack frames appearing when paused on line (3) unless I remove all source mappings from line (2). With those removed the stack traces from errors and the debugging experience in chrome devtools with source maps enabled will regress.

@grouma Let's do some brainstorming to come up with a more targeted way we can detect async methods and hide the unwanted stack frame. Add a list of source maps to ignore in the metadata? Other ideas?

@grouma
Copy link
Member

grouma commented Jan 13, 2021

When collecting async frames we do actually remove duplicate async markers. We could potentially add some similar logic in that class. What would be the signal though? Is it enough to remove back to back frames with the exact same location?

@nshahan
Copy link
Contributor Author

nshahan commented Jan 13, 2021

When collecting async frames we do actually remove duplicate async markers. We could potentially add some similar logic in that class. What would be the signal though?

I'm not sure, maybe we should hack on this and brainstorm.

Is it enough to remove back to back frames with the exact same location?

I don't think that's the answer because the duplicated frames are not identical because they actually map to different lines.

Screen Shot 2021-01-12 at 4 47 50 PM

@vsmenon
Copy link
Member

vsmenon commented Jan 19, 2021

Any update?

@grouma
Copy link
Member

grouma commented Jan 19, 2021

To properly resolve this issue we likely need to make use of the DDC metadata files. This issue is very similar to dart-lang/webdev#1179. We won't get to it in this quarter but we intend to have an OKR around enhanced inspection next quarter. This enhanced inspection will leverage additional metadata within the DDC metadata files instead of relying on various heuristics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. P2 A bug or feature request we're likely to work on web-dev-compiler web-triage-1 priority assigned
Projects
None yet
Development

No branches or pull requests

5 participants