Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690873005:
Enable causal stacktrace in kernel (Closed)
|
Created:
3 years, 10 months ago by jensj Modified:
3 years, 10 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionEnable causal stacktrace in kernel
R=kmillikin@google.com, kustermann@google.com
Committed: https://github.com/dart-lang/sdk/commit/b29829660bcdee7eb394e6fd4635ff7a6a77ef01
Committed: https://github.com/dart-lang/sdk/commit/26b6ecdf634bc89bb326d8be2cbee407a21cbf80
Patch Set 1 #
Total comments: 10
Patch Set 2 : Changes based on comments #Patch Set 3 : Change kernel function 'debuggable' field to an 'originalAsyncMarker' field, use it to set function… #
Total comments: 12
Patch Set 4 : Addressed comments #
Total comments: 1
Patch Set 5 : Addressed more comments #Patch Set 6 : function.set_is_inlinable to make precompiled builds work too #
Messages
Total messages: 21 (5 generated)
jensj@google.com changed reviewers: + kmillikin@google.com, kustermann@google.com
Basically a first-stab at "kernel-isation" of https://github.com/dart-lang/sdk/commit/a0ee5b24db5420d8256f22e89561d13038f3320c parser.cc and flow_graph_builder.cc - Asserts will be hit in debug-mode. - In source-version some functions is set as not-inlinable. I haven't done that yet for this. Needed - among, I'm guessing, other things - for service tests to pass...
LGTM https://codereview.chromium.org/2690873005/diff/1/pkg/kernel/lib/transformati... File pkg/kernel/lib/transformations/continuation.dart (right): https://codereview.chromium.org/2690873005/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/continuation.dart:267: // :async_stack_trace = _asyncStackTraceHelper(); nit: double space after = https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc File runtime/vm/kernel_to_il.cc (right): https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc#... runtime/vm/kernel_to_il.cc:2577: instructions <<= call_async_clear_thread_stack_trace; Could you rewrite this using our helper functions, something like this: // We are returning from an asynchronous closure. Before we do that, be // sure to clear the thread's asynchronous stack trace. const Function& target = Function::ZoneHandle( Z, I->object_store()->async_clear_thread_stack_trace()); instructions += StaticCall(TokenPosition::kNoSource, target, 0); ? https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc#... runtime/vm/kernel_to_il.cc:3267: intptr_t current_context_depth = context_depth_; nit: const (maybe also from the code you copied it from) https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc#... runtime/vm/kernel_to_il.cc:3272: // Fetch the :async_stack_trace variable and store it into the thread. Could you add empty lines before comments like this, would make it much easier to read. (also below) https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc#... runtime/vm/kernel_to_il.cc:3291: body = instructions + body; Could you also use StaticCall here i.e. replace most of this with const Function& target = Function::ZoneHandle(Z, I->object_store()->async_set_thread_stack_trace()); LocalVariable* async_stack_trace_var = scope->LookupVariable(Symbols::AsyncStackTraceVar(), false); instructions += LoadLocal(async_stack_trace_var); instructions += PushArgument(); instructions += StaticCall(TokenPosition::kNoSource, target, 1); ?
jensj@google.com changed reviewers: + vegorov@google.com
Addressed comments. vm/regress_28325_test passes by mistake (looking for ':12' in stacktrace, and the output contains '_startIsolate.<anonymous closure> (dart:isolate:1230:9)') vm/causal_async_exception_stack_test does not pass because it is (e.g.) looking for 'main.inner.deep', whereas in kernel it's actually called 'main.async_op.inner.async_op.deep.async_op' Added Slava as reviewer as he seemed to have an opinion. PTAL. https://codereview.chromium.org/2690873005/diff/1/pkg/kernel/lib/transformati... File pkg/kernel/lib/transformations/continuation.dart (right): https://codereview.chromium.org/2690873005/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/continuation.dart:267: // :async_stack_trace = _asyncStackTraceHelper(); On 2017/02/14 14:45:53, kustermann wrote: > nit: double space after = Done. https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc File runtime/vm/kernel_to_il.cc (right): https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc#... runtime/vm/kernel_to_il.cc:2577: instructions <<= call_async_clear_thread_stack_trace; On 2017/02/14 14:45:53, kustermann wrote: > Could you rewrite this using our helper functions, something like this: > > // We are returning from an asynchronous closure. Before we do that, be > // sure to clear the thread's asynchronous stack trace. > const Function& target = Function::ZoneHandle( > Z, I->object_store()->async_clear_thread_stack_trace()); > instructions += StaticCall(TokenPosition::kNoSource, target, 0); > > ? Done. https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc#... runtime/vm/kernel_to_il.cc:3267: intptr_t current_context_depth = context_depth_; On 2017/02/14 14:45:53, kustermann wrote: > nit: const (maybe also from the code you copied it from) Done. https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc#... runtime/vm/kernel_to_il.cc:3272: // Fetch the :async_stack_trace variable and store it into the thread. On 2017/02/14 14:45:53, kustermann wrote: > Could you add empty lines before comments like this, would make it much easier > to read. (also below) Done. https://codereview.chromium.org/2690873005/diff/1/runtime/vm/kernel_to_il.cc#... runtime/vm/kernel_to_il.cc:3291: body = instructions + body; On 2017/02/14 14:45:53, kustermann wrote: > Could you also use StaticCall here > > i.e. replace most of this with > > const Function& target = Function::ZoneHandle(Z, > I->object_store()->async_set_thread_stack_trace()); > > LocalVariable* async_stack_trace_var = > scope->LookupVariable(Symbols::AsyncStackTraceVar(), false); > > instructions += LoadLocal(async_stack_trace_var); > instructions += PushArgument(); > instructions += StaticCall(TokenPosition::kNoSource, target, 1); > ? Done.
LGTM
Description was changed from ========== Enable causal stacktrace in kernel ========== to ========== Enable causal stacktrace in kernel R=kustermann@google.com Committed: https://github.com/dart-lang/sdk/commit/b29829660bcdee7eb394e6fd4635ff7a6a77ef01 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as b29829660bcdee7eb394e6fd4635ff7a6a77ef01 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Enable causal stacktrace in kernel R=kustermann@google.com Committed: https://github.com/dart-lang/sdk/commit/b29829660bcdee7eb394e6fd4635ff7a6a77ef01 ========== to ========== Enable causal stacktrace in kernel R=kustermann@google.com Committed: https://github.com/dart-lang/sdk/commit/b29829660bcdee7eb394e6fd4635ff7a6a77ef01 ==========
So this thing was reverted because tests failed in debug mode (I don't know why I forgot about that, I even pointed it out in a comment above :x). Anyways, Martin suggested setting the function modifiers (async, async* etc) so I've done that. Seems to work fine. PTAL (mind the rebase though).
On request I've split the extra into another CL instead (https://codereview.chromium.org/2697193008/).
LGTM with comments addressed. https://codereview.chromium.org/2690873005/diff/40001/runtime/vm/kernel_to_il.cc File runtime/vm/kernel_to_il.cc (right): https://codereview.chromium.org/2690873005/diff/40001/runtime/vm/kernel_to_il... runtime/vm/kernel_to_il.cc:724: // TODO(28777): Either remove the variable here, or update the dart side The comment is confusing. It's not entirely clear what "the dart side" is. I guess that means, the Dart->Kernel translator, but I wouldn't assume here that it's implemented in Dart and I'd just be explicit. https://codereview.chromium.org/2690873005/diff/40001/runtime/vm/kernel_to_il... runtime/vm/kernel_to_il.cc:2568: function.name() == Symbols::AsyncOperation().raw()) { We should set the function to be correctly IsAsyncClosure or IsAsyncGenClosure and use that instead of the name. https://codereview.chromium.org/2690873005/diff/40001/runtime/vm/kernel_to_il... runtime/vm/kernel_to_il.cc:3261: dart_function.name() == Symbols::AsyncOperation().raw()) { Also here, I'd rather not use the name if we don't have to. https://codereview.chromium.org/2690873005/diff/40001/tests/language/language... File tests/language/language_kernel.status (right): https://codereview.chromium.org/2690873005/diff/40001/tests/language/language... tests/language/language_kernel.status:47: # vm/regress_28325_test: RuntimeError # Issue 28055. Passes by mistake. Uncomment the line and give it an expectation of Pass? https://codereview.chromium.org/2690873005/diff/40001/tests/language/language... tests/language/language_kernel.status:207: vm/causal_async_exception_stack_test: RuntimeError I think this might be caused by not setting the parent function of SyncYielding functions to be correctly sync*/async/async*. That is used to hide the names of the generated functions.
https://codereview.chromium.org/2690873005/diff/40001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/continuation.dart (right): https://codereview.chromium.org/2690873005/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/continuation.dart:273: statements.add(asyncStackTraceVariableAssign); We could actually just create the variable here and make this call in C++ (under the FLAG_...). Otherwise we might regress performance compared to the old VM pipeline if the FLAG_... is not set.
Thanks. Comments addressed. PTAL. https://codereview.chromium.org/2690873005/diff/40001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/continuation.dart (right): https://codereview.chromium.org/2690873005/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/continuation.dart:273: statements.add(asyncStackTraceVariableAssign); On 2017/02/22 11:08:25, kustermann wrote: > We could actually just create the variable here and make this call in C++ (under > the FLAG_...). > > Otherwise we might regress performance compared to the old VM pipeline if the > FLAG_... is not set. Attempt made. https://codereview.chromium.org/2690873005/diff/40001/runtime/vm/kernel_to_il.cc File runtime/vm/kernel_to_il.cc (right): https://codereview.chromium.org/2690873005/diff/40001/runtime/vm/kernel_to_il... runtime/vm/kernel_to_il.cc:724: // TODO(28777): Either remove the variable here, or update the dart side On 2017/02/22 10:42:17, Kevin Millikin (Google) wrote: > The comment is confusing. It's not entirely clear what "the dart side" is. I > guess that means, the Dart->Kernel translator, but I wouldn't assume here that > it's implemented in Dart and I'd just be explicit. I have attempted to move the call to _asyncStackTraceHelper into the C++ code instead (thus hidden behind the FLAG_causal_async_stacks). I've thus removed the comment. https://codereview.chromium.org/2690873005/diff/40001/runtime/vm/kernel_to_il... runtime/vm/kernel_to_il.cc:2568: function.name() == Symbols::AsyncOperation().raw()) { On 2017/02/22 10:42:18, Kevin Millikin (Google) wrote: > We should set the function to be correctly IsAsyncClosure or IsAsyncGenClosure > and use that instead of the name. Done. https://codereview.chromium.org/2690873005/diff/40001/runtime/vm/kernel_to_il... runtime/vm/kernel_to_il.cc:3261: dart_function.name() == Symbols::AsyncOperation().raw()) { On 2017/02/22 10:42:17, Kevin Millikin (Google) wrote: > Also here, I'd rather not use the name if we don't have to. Done. https://codereview.chromium.org/2690873005/diff/40001/tests/language/language... File tests/language/language_kernel.status (right): https://codereview.chromium.org/2690873005/diff/40001/tests/language/language... tests/language/language_kernel.status:47: # vm/regress_28325_test: RuntimeError # Issue 28055. Passes by mistake. On 2017/02/22 10:42:18, Kevin Millikin (Google) wrote: > Uncomment the line and give it an expectation of Pass? Done. https://codereview.chromium.org/2690873005/diff/40001/tests/language/language... tests/language/language_kernel.status:207: vm/causal_async_exception_stack_test: RuntimeError On 2017/02/22 10:42:18, Kevin Millikin (Google) wrote: > I think this might be caused by not setting the parent function of SyncYielding > functions to be correctly sync*/async/async*. That is used to hide the names of > the generated functions. It seems you are correct. It passes now.
I don't think we should even have the variable declaration in the Kernel IR, it's only used in the VM's implementation.
https://codereview.chromium.org/2690873005/diff/60001/runtime/vm/kernel_to_il.cc File runtime/vm/kernel_to_il.cc (right): https://codereview.chromium.org/2690873005/diff/60001/runtime/vm/kernel_to_il... runtime/vm/kernel_to_il.cc:6227: if (FLAG_causal_async_stacks && You can move this code to FlowGraphBuilder::BuildGraphOfFunction if (dart_function.hasOriginalAysncMarker()) { // call async helper // store in :async_stack var. }
Comments addressed. Kevin talked about putting NULL into the :async_stack_trace variable, which I didn't do. I don't think it is necessary? PTAL.
As an additional comment, all* tests seems to pass. * I ran: python tools/test.py -mrelease cc standalone service lib utils corelib isolate language co19 python tools/test.py -mrelease dart2js python tools/test.py -mrelease --checked -t60 --use-sdk pkg/kernel python tools/test.py -cdartk -mrelease kernel utils language co19 python tools/test.py -cdartk -mdebug kernel python tools/test.py -mrelease --time --use-sdk --reset-browser-configuration --checked dart2js python tools/test.py -cdartkp -rdart_precompiled -mrelease kernel python tools/test.py -cdartkp -rdart_precompiled -mdebug kernel python tools/test.py -cdartk -mdebug language/async python tools/test.py -cdartk -mdebug language/caus python tools/test.py -cdartkp -rdart_precompiled -mrelease language/async python tools/test.py -cdartkp -rdart_precompiled -mrelease language/caus python tools/test.py -cdartkp -rdart_precompiled -mdebug language/async python tools/test.py -cdartkp -rdart_precompiled -mdebug language/caus so I did't actually test all precompiled stuff for instance, but at least some :)
LGTM
Description was changed from ========== Enable causal stacktrace in kernel R=kustermann@google.com Committed: https://github.com/dart-lang/sdk/commit/b29829660bcdee7eb394e6fd4635ff7a6a77ef01 ========== to ========== Enable causal stacktrace in kernel R=kmillikin@google.com, kustermann@google.com Committed: https://github.com/dart-lang/sdk/commit/b29829660bcdee7eb394e6fd4635ff7a6a77ef01 Committed: https://github.com/dart-lang/sdk/commit/26b6ecdf634bc89bb326d8be2cbee407a21cbf80 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 26b6ecdf634bc89bb326d8be2cbee407a21cbf80 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
