Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Finish lazy stack trace support by implementing async* support and fixing line numbers #39525

Closed
3 of 4 tasks
mkustermann opened this issue Nov 26, 2019 · 4 comments
Closed
3 of 4 tasks

Finish lazy stack trace support by implementing async* support and fixing line numbers #39525

mkustermann opened this issue Nov 26, 2019 · 4 comments
Assignees
@mkustermann
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Milestone
D28 Release

Comments

@mkustermann
Copy link
Member

mkustermann commented Nov 26, 2019 •
edited by ghost
Loading

The cl/122644 adds the --lazy-async-stacks. After the CL lands we still have to do two follow up changes

  • Ensure we have correct line numbers for async frames
  • Implement support for async* functions
  • Investigate why the test fails on dartk-optcounter-linux-release-x64
  • Start rolling out the lazy stack trace support

The support for async* should be analogous to async:

  • Assign fixed context index to _AsyncStarStreamController :controller
  • Add pragma("vm:entry-point") annotations to used fields
  • In stack trace generation code, follow the metadata to find the listener

The metadata should be available via:

_AsyncStarStreamController<T>
    StreamController<T> controller
      _StreamController _varData (if `_state == _STATE_SUBSCRIBED`)
          _ControllerSubscription<T>/_BufferingStreamSubscription.<T>_onData
             _StreamIterator._onData (validate it's a _StreamIterator._onData tearoff, need to get "this" from tearoff context)
                 _StreamIterator._stateData (if is _Future)
                      _Future.... (the regular code to find awaiter from _Future)

/cc @mraleph
@mkustermann mkustermann added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Nov 26, 2019
@mkustermann mkustermann assigned ghost Nov 26, 2019
@ghost
Copy link

ghost commented Nov 26, 2019

Note: This is a follow-up to #37668

dart-bot pushed a commit that referenced this issue Dec 4, 2019
[SDK] Adds --lazy-async-stack support for async*.
f9327d3 
Bug: #39525
Change-Id: I53cd334243649901ea8e0f9799d9f41c126e3627
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/126729
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Dec 18, 2019
[SDK] Switch to isSync to determine if async func. is running sync-as…
3764928 
…ync.

Bug: #39525
Change-Id: I1e23a726bf0fbff2c02891e25c714ea599330c47
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/128666
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Dec 18, 2019
[SDK] Fixes missing stack trace line numbers.
6fcd1de 
CodeSourceMapBuilder::NoteDescriptor(..) would previously only emit
CSM entries if its stack_traces_only_ flag was set, which
FlowGraphCompiler would only do if compiled in PRODUCT mode:
runtime/vm/compiler/backend/flow_graph_compiler.cc:162

Bug: #39525
Change-Id: I78c56a18a5a95ef3c8c37a2d7eae6ab612e6674f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/127464
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Dec 20, 2019
[SDK] Switch to is_sync to identify sync/async running.
886615d 
This should address the regression introduced by https://dart-review.googlesource.com/c/sdk/+/124988

Bug: #39525
Change-Id: Id163b649bdd0363297c186559fa84ff87f908e4b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129062
Reviewed-by: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Clement Skau <cskau@google.com>
dart-bot pushed a commit that referenced this issue Dec 20, 2019
@mkustermann
Revert "[SDK] Switch to is_sync to identify sync/async running."
72f4a83 
This reverts commit 886615d.

Reason for revert:

There was an unexpected slowdown in some async benchmarks, e.g. Calls.AwaitAsyncCall.

Will revert for now and investigate next year.


Original change's description:
> [SDK] Switch to is_sync to identify sync/async running.
> 
> This should address the regression introduced by https://dart-review.googlesource.com/c/sdk/+/124988
> 
> Bug: #39525
> Change-Id: Id163b649bdd0363297c186559fa84ff87f908e4b
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129062
> Reviewed-by: Clement Skau <cskau@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>
> Commit-Queue: Clement Skau <cskau@google.com>

TBR=kustermann@google.com,cskau@google.com

Change-Id: I5cda795cbccc01f22e0f8192473c171a4e9fca4b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: #39525
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129285
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Jan 6, 2020
@mkustermann
[vm] Read causal-async-stacks/lazy-async-stacks from snapshot (instea…
1669769 
…d of requiring AOT runtime to pass same flags as in compiler)

We already do this for "use-bare-instructions" and "dwarf-stack-traces". This
CL does the same for "causal-async-stacks" and "lazy-async-stacks".

Issue #39525

Change-Id: I44136299858b3b72e8a3a176234f5051d6b61c1d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130364
Reviewed-by: Teagan Strickland <sstrickl@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
@mkustermann mkustermann added this to the D28 Release milestone Jan 7, 2020
dart-bot pushed a commit that referenced this issue Jan 22, 2020
[SDK] Fixes stack trace service API tests.
e047ece 
Fixes an issue where stack['asyncCausalFrames'] would be populated in
if --lazy-async-stacks was set.
The field should be filled iff the stack contains an async function -
regardless of whether it's sync-async.

Bug: #39525
Change-Id: Ia68f113402962c07a1e9a38ea9320b44140241e4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132820
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Jan 24, 2020
[SDK] Fixes async* stack unwinding.
da1e42a 
For `foo() async* {}` frames we find the "caller" by finding out
what closure is registered as listener on the _AsyncStreamController.

There can be two cases:

a) The caller does a regular `foo().listen((_) {})`:
  The stack trace will have the closure as the caller and unwinding stops.

b) The caller uses 'await for (... foo())':
  In this case the listener will be a StreamIterator.

This CL changes our unwinding code to get the awaiter of `await it.moveNext()`.


Bug: #39525
Change-Id: I13f76025a15682aaf55fd968088fc2059982d842
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132841
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Jan 24, 2020
[SDK] Fixes service/break_on_default_constructor_test for --lazy-asyn…
173ae21 
…c-stacks.

With this fix, all 'service' target tests now pass with --lazy-async-stacks
on by default for {dartk,dartkp,dartkb-mixed}-{debug,product,release}.

Bug: #39525
Change-Id: I2c21f6d2f054fdbb261eb8dfbf422b3950cf9648
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132903
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Jan 31, 2020
[SDK] Fixes remaining issues with --lazy-async-stacks on.
26ada43 
- Handle bytecode for e.g. dartkb-simarm64 does not have source position.
- Use existing GetCallerSp() instead of working across frames.
- Nit: Adds clarifying comments to test.
- Nit: Updates name of non-async-stack test to clarify flags used.

Tested:
- CQ with --lazy-async-stacks on by default.
- vm/dart/causal_stacks and language_2/vm/causal_async_exception_stack_test with current flags.

Bug: #39525
Change-Id: Ie6581a734cdcafbd4fb641bd86bffc03ed241532
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/133063
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
@ghost
Copy link

ghost commented Jan 31, 2020

As for the dartk-optcounter-linux-release-x64 failures:

Testing locally I see dartk-optcounter-linux-*-x64 failing vm/dart/causal_stacks/async_throws_stack_test tests with --causal-async-stacks.
The stacktraces incorrect labels sync-async frames async-async and inserts bogus <asynchronous suspension>.

My understanding of what is happening is that the optimizing causes certain frames to disappear - specifically I see a diff optcounter and non-optcounter showing _Closure.call and sometimes even _AsyncAwaitCompleter.start disappearing.
This causes issues for stack_trace.h:CheckAndSkipAsync(..) which checks for these frames to determine if we're running sync-async.
If neither of these frames are seen we're (incorrectly) assumed to be async-async.

@ghost ghost assigned mkustermann Jan 31, 2020
@ghost
Copy link

ghost commented Jan 31, 2020

[Co-assigning mkustermann who mentioned he might look into the last item on the check list.]

@mkustermann
Copy link
Member Author

In terms of functionality, there's no more actual work planned on the lazy async stacks. So I'll close this issue.

There's still some non-feature work to do, mainly to get our embedders opted in and later on change the default.

@mkustermann mkustermann mentioned this issue May 8, 2020
Closed
@ghost ghost mentioned this issue Jun 19, 2020
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mkustermann mkustermann

Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Milestone
D28 Release
Development

No branches or pull requests
1 participant
@mkustermann