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.

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

Enable new "Profile" and "Allocation Tracing" sub-tabs in Memory screen #4523

Merged
merged 3 commits into from Sep 26, 2022

Conversation

bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Sep 26, 2022

Also removes the old "Allocations" tool and its relevant code.

Also removes the old "Allocations" tool and its relevant code.
@polina-c
Copy link
Contributor

polina-c commented Sep 26, 2022

Please rename columns to be consistent with diff tab:
Size / Internal / External - > Total Shallow / Shallow Dart / Shallow Native

And in the documentation: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/screens/memory/panes/allocation_profile/ALLOCATION_PROFILE.md

For total shallow reference this tooltip:

@polina-c
Copy link
Contributor

The bug is still here: #4484
Can you fix it?

@polina-c
Copy link
Contributor

And sorting by instances does not seem to work too:
Screen Shot 2022-09-26 at 2 45 03 PM

@polina-c
Copy link
Contributor

Tooltips for columns are the same in every group. We need to either to add tooltips for column groups (seems to be cleaner) or to have little different tooltips for the columns themselves.

@bkonyi
Copy link
Contributor Author

bkonyi commented Sep 26, 2022

@polina-c I'm going to follow up on this change with additional PRs that clean things up. Didn't want to put them all in one PR.

@bkonyi bkonyi merged commit fff97df into flutter:master Sep 26, 2022
@bkonyi bkonyi deleted the enable_new_memory_pages branch September 26, 2022 22:57
@bkonyi
Copy link
Contributor Author

bkonyi commented Sep 27, 2022

The bug is still here: #4484 Can you fix it?

And sorting by instances does not seem to work too: Screen Shot 2022-09-26 at 2 45 03 PM

Both of these are fixed by #4532.

@bkonyi
Copy link
Contributor Author

bkonyi commented Sep 27, 2022

Please rename columns to be consistent with diff tab: Size / Internal / External - > Total Shallow / Shallow Dart / Shallow Native

And in the documentation: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/screens/memory/panes/allocation_profile/ALLOCATION_PROFILE.md

For total shallow reference this tooltip:

"Shallow Native" isn't necessarily correct as it's non-Dart memory and only as accurate as the values provided by the embedder (see the specification here). There's no requirement for the provided value to be deep/shallow, or for the embedder to provide any value (e.g., the Flutter engine no longer reports image data sizes via this field as it manages the lifecycle of images independently from the VM's garbage collector).

I'm also not convinced that it's necessary to change the headers to be this verbose when we can just update the tooltips to better explain that these are shallow memory values, especially when there's no retained memory column in the same context. I propose that we use the following instead:

  • "Size" -> "Total Size" Tooltip: "The sum of the object type's total shallow memory consumption in the Dart heap and associated external (e.g., non-Dart heap) allocations"
  • "Internal" -> "Dart Heap" Tooltip: "$shallowSizeColumnTooltip"
  • "External" -> "External" Tooltip: "Non-Dart heap allocated memory associated with an object type"

@bkonyi
Copy link
Contributor Author

bkonyi commented Sep 27, 2022

Tooltips for columns are the same in every group. We need to either to add tooltips for column groups (seems to be cleaner) or to have little different tooltips for the columns themselves.

I don't think this is necessary, at least at this point, since only the Total column group will typically be displayed for the vast majority of users.

Also, I really appreciate the attention to detail, but it's better for my workflow to have new bugs filed for issues not directly related to the current PR and referenced in the PR comments and/or assigned to directly to me as requiring a follow up. That way we can keep discussions centralized in the issue tracker and not tied directly to a potentially closed / abandoned PR that can easily be lost, and I don't start encountering scope creep for individual PRs :-)

@kenzieschmoll
Copy link
Member

"Size" -> "Total Size" Tooltip: "The sum of the object type's total shallow memory consumption in the Dart heap and associated external (e.g., non-Dart heap) allocations"
"Internal" -> "Dart Heap" Tooltip: "$shallowSizeColumnTooltip"
"External" -> "External" Tooltip: "Non-Dart heap allocated memory associated with an object type"

SGTM - does "Dart Heap" + "External" for an object = "Total Size"?

@bkonyi
Copy link
Contributor Author

bkonyi commented Sep 27, 2022

SGTM - does "Dart Heap" + "External" for an object = "Total Size"?

That's correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants