Elide tree walks #48413
Elide tree walks #48413
Conversation
fluttergithubbot
added
the
framework
| } else if (frameLine.packageScheme == 'package' && frameLine.package == 'flutter') { | ||
| if (visited.add('flutter/${frameLine.packagePath}:${frameLine.line}:${frameLine.column}')) { | ||
| if (skippedFromFlutterCount > 0) { | ||
| result.add('(elided $skippedFromFlutterCount frame${skippedFromFlutterCount == 1 ? '' : 's'} from package:flutter)'); |
There was a problem hiding this comment.
Maybe: 'elided 2 revisited frames ...' or something similar so that there's some explanation of what was left out.
There was a problem hiding this comment.
Talked offline. We should figure out what the best thing for a user is here. It might just be chopping the stack N frames of flutter code after user code.
There was a problem hiding this comment.
Would IDE users ever want to expand out the elided frames? If so, it could be worth it to expose the elided frames with enough structure that IDEs can expose expanding out the elided frames.
The ideal solution would be that an IDE received this data with enough structure to expand and collapse blocks of elided frames if requested by an IDE user who wants to see the elided frames.
I do like placing the logic of what frames should start out elided in package:flutter instead of having intellij, devtools, and VSCode create their own logic about what frames to elide when displaying flutter error messages.
It would be even better if consistent frame black boxing was also applied to stack traces displayed by IDEs but that would be a hard problem to solve.
There was a problem hiding this comment.
I personally don't think it's ever useful to see all of these frames. Or, if it is, it's so rare that it's worth just instrumenting the code differently in the framework by hand.
Do the IDEs use the defaultStackFilter, or do they replace it?
There was a problem hiding this comment.
IDEs currently use defaultStackFilter. If eliding frames gets aggressive enough that users sometimes need to see the frames, we could revisit.
There was a problem hiding this comment.
If people need to debug something tricky there's always the option of adding breakpoints/break-on-all-exceptions where they'd get the full stack in the debugging tools.
In VS Code we do collapse multiple frames from SDKs that one (eg. "<105 frames from Flutter SDK>") but you can click-to-expand.
I suspect few people would miss these frames being removed from here (in VS Code at least) if they're still there in the debugger call stack.
There was a problem hiding this comment.
Debuggers are more or less useless when debugging this code, because stepping through what we do even for a single frame takes hours. Stack inspection and printf-debugging is really the only reasonable way I've found to debug errors deep in the framework.
There was a problem hiding this comment.
Ah, fair enough! I thought break-on-all-exceptions would break you in a good place but I've really no experience trying to debug these sorts of issues so I'll defer to the experts :-)
Jacob's original idea sounds good though - if all stack frames are provided to the IDE in a structured way with flags on which ones are reasonable to elide, then for IDEs that have an easy way to collapse them (but still make them accessible - similar to VS Code's call stack), that might be best of both worlds (for locations where interaction isn't possible, the whole list could be shown, or have a user preference - similar to how we let users device whether to have Framework libraries debuggable or not).
There was a problem hiding this comment.
Generally the stack traces I'm trying to divine from are on GitHub, no IDEs in sight. :-)
|
@InMatrix - what do you think of this? @DanTup @jacob314 @devoncarew - will this negatively impact IDEs? |
|
Stack traces from the framework generally have this shape:
We could probably safely omit all Flutter frames after the first |
|
Eliding everything after the first mount would look something like: |
|
Supposing that |
|
This breaks some tests that are asserting against error messgaes - I'll update the tests if there's consensus that this is a good approach. |
|
It's a bit more subtle than the discussion above suggests. It's not only Flutter code in these iterations. For example, users can create their own Elements or RenderObjects and then their code will be present in these loops. It's critical that users be able to see this information when debugging their own logic. The iterations are not perfectly repeated. For example, some widgets have custom elements, and they might have their own mounting logic. When you're doing layout, every node has its own performLayout method. Thus every one out of every 3 or 4 frames is actually subtly different in a potentially useful way. I've definitely often made use of this information. What we could do is recognize specific patterns (building, mounting, unmounting widgets; layout or paint of render objects; semantics; etc) and replace the relevant parts of the stack with a clean description of what's happening, e.g. "laying out render objects: RenderStack (stack.dart:302), RenderDecoratedBox (proxy_box.dart:201), RenderFlex (flex.dart:585), RenderFlex (flex.dart:585)" or something. |
| '#348 _Timer._runTimers (dart:isolate-patch/timer_impl.dart:384:19)', | ||
| '#349 _Timer._handleMessage (dart:isolate-patch/timer_impl.dart:418:5)', | ||
| '#350 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:174:12)', | ||
| '(elided 330 frames from package flutter)' |
There was a problem hiding this comment.
here in particular i think we are eliding way too much valuable information. It'd be better if between #9 and #348 we said something like "Inflated 28 elements". Even more than that though, because consider frames #21 or #60 for example, notice how those are different from the rest of the pattern.
I definitely wouldn't elide frames #333 to #339 inclusive. I think on the other hand we should elide #348 to #350, that we don't is just an oversight in the current code.
BTW we should make these tests verify the output on real stacks generated by the test, because we want to catch future regressions where Dart or the Flutter framework change structure and the logic here bitrots again.
There was a problem hiding this comment.
I don't think we can properly handle user code this way, but if it's a custom widget we wouldn't omit it here because that would live outside of package:flutter.
Part of the problem is a stack trace line just doesn't give us enough information (particularly on web, where we don't even get the class name). What might be possible is iterating from the bottom up until we hit a mount call from framework.dart, then group those together until we get to the one before we hit user code (and repeat if there's a custom element in user code).
There was a problem hiding this comment.
It's fine if we do a bad job on Web because Web doesn't give us enough yet. We should aim for doing the best we can on each platform we support, and not get tied down by the least common denominator.
There was a problem hiding this comment.
we could have pattern matchers for specific stack patterns we know about. if we do that, then there should be a way for these patterns to get registered by the bindings, so that each layer can register its own matchers.
|
@Hixie I think the stack you're thinking of is different from what I'm used to seeing. Probably a throw during layout? In any case, I tried just cleaning up the But it's still looking too verbose to me. |
|
The significant difference being that all of these calls are coming from the same file and the same two or three generic framework elements - so nothing nice about "oh, this one was for a stack, this one was for a row" etc. |
|
For the specific error example @dnfield showed earlier in this PR, nothing in the stack is useful or necessary for the user to debug the error. If the lines above the stack are not enough for the user to understand the error, the user is more likely to google the error message than digging through the stack. It would be helpful to use an example where the stack actually has information the user will need to debug the error. |
|
Perhaps rather than dumping the full Flutter stack, we should dump the truncated stack (just the first few frames in Flutter land), and then some kind of helpful summary of the current state of render/element/layer/semantics tree stuff? |
Sure, but how can we distinguish those from cases where they are necessary?
Can we collapse identical lines in that stack too? |
|
e.g.: Though I just noticed that where this says ComponentElement it should really say either StatefulElement or StatelessElement, to be more useful... |
|
If you recognized those lines as matching the "mounting elements" patterns (see my earlier suggestion for defined pattern matchers), you could go further: e.g.: |
|
Or maybe (or whatever the count is, I didn't actually count it.) If there was a special element involved, then it would still show its stack because it wouldn't match the pattern. |
Layout has similar patterns and similar needs, sure. Paint, also. The widget framework has a number of different cases where there's tree iteration that has patterns we could recognize, like the one you've listed here but also things like exceptions during parent data application, global key activation, dependency changes, etc. |
I'm genuinely curious about the cases when information in the stack is necessary. Without anyone in the thread presenting such as case, it makes whoever hasn't experienced such cases themselves very difficult to participate in this discussion and be convinced that the final resolution is reasonable (i.e., striking a good balance between cost and benefit). |
|
I like @Hixie's idea of having various bindings register stack recognition routines. I think that could get us something more useful here without completely truncating the stack. |
|
Fyi @kenzieschmoll, @bkonyi. To integrate into the flame chart display it would probably be simpler to expose registering the stack recognition routines in |
|
@InMatrix For example at home I'm experimenting with a library which introduces a bunch of new RenderObject subclasses and corresponding widgets, as well as a different set of subclasses of RenderObjectElement. When I messed up my logic in my RenderObjectElement subclass, I had to figure out exactly what was going wrong by studying the stacks I was getting. Similarly we were recently debugging an issue with GlobalKey reparenting, where the precise order of activate/deactivate/etc matters to figuring out what's wrong. |
| const PartialStackFrame componentElementFristBuild = PartialStackFrame(className: 'ComponentElement', method: '_firstBuild', packagePath: 'src/widgets/framework.dart'); | ||
| const PartialStackFrame componentElementMount = PartialStackFrame(className: 'ComponentElement', method: 'mount', packagePath: 'src/widgets/framework.dart'); | ||
| const PartialStackFrame statefulElementFristBuild = PartialStackFrame(className: 'StatefulElement', method: '_firstBuild', packagePath: 'src/widgets/framework.dart'); | ||
| const PartialStackFrame singleChildMount = PartialStackFrame(className: 'SingleChildRenderObjectElement', method: 'mount', packagePath: 'src/widgets/framework.dart'); |
There was a problem hiding this comment.
nit: it would be nice if these filters could include package:flutter in the package path to eliminate the small chance of false positives.
|
|
||
| /// The method name for this frame line. | ||
| /// | ||
| /// On web, private methods are wrapped with `[]`. |
|
I'm going to land this on green - @Hixie if any concerns remain we can continue to iterate on it. |
|
This pull request is not suitable for automatic merging in its current state.
|
|
Looks like Cirrus is unhappy on this one now. |
|
Should be fixed on latest commit. I forgot to update the regex string in a test. |
|
This pull request is not suitable for automatic merging in its current state.
|
This reverts commit 46ccd08.
This reverts commit effc019.
10 participants
Description
Elides repetitive tree walking related frames from stack traces in Flutter.
For example, a stack trace resulting from a bad deeply nested widget now prints out 29 lines instead of over 300.
I'm not sure if we want to have a debug flag for disabling this - I personally have never cared about how many tree walks happened when debugging framework issues though. /cc @chunhtai @Piinks @goderbauer for feedback on that.
@Hixie @a14n @zanderso @jonahwilliams probably have thoughts on this too.
Related Issues
Fixes #9329
Tests
I added the following tests:
Test asserting defaultStackFilter filters a huge ugly stack down.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.