Refactoring and fixing of HeapExplorer #12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Introduced the `Option<A>` type to represent values that might not be there. * Replaced values representing failure (like `-1`) with `Option` to indicate the possibility of failure. * Replaced possibly-invalid types with `Option`s. Now if you if have an instance - it's guaranteed to be valid. * Replaced magic values (like using positive integers for managed object indexes and negative integers for static object indexes) with proper semantic types to help with code clarity. * Replaced dangerous numeric casts that could under/overflow with typesafe alternatives (`PInt` positive 32bit integer type) that guarantee no such things can happen. * Replaced instances of `List` which were being used as stacks with `Stack`. * Replaced as many fields in objects as possible with readonly ones to indicate that we have no intent of changing them after the object is constructed. This is a part of 'objects should stay valid after construction' philosophy. * Added logging for potentially dangerous operations (like int -> uint). * Replaced primitive loop guards with cycle tracking that tracks seen objects. This eliminates the false positives and errors out sooner if we actually have an error. * Replaced places where things got cancelled silently with emitting warnings / errors. Failures should never be silent. * Fixed algorithm that checks if a type contains reference types: it didn't account for nested value-types which eventually have reference types in them and only looked for reference types in the first level. * Fixed bad lookups in `PackedCoreTypes`. Also made `PackedCoreTypes` to be fully initialized, without any missing entries. * Added a warning about the inability to crawl the `[ThreadStatic]` variables. * Improved search speed by: * caching search results. * downcasing everything before the search and then using fast ordinal string comparisons instead of slower `OrdinalIgnoreCase` comparisons. * Improved documentation by adding new docstrings and converting existing docstrings into proper XMLDocs. Now IDEs and tools can properly render the documentation. * Rewrote a part of code using C# 7.3 (supported since Unity 2019) to enhance readability. * bumped version to 4.1.2 * Added capability to copy the root path as text in "Find GC roots" window. * Added information about source fields that are pointing to the object in "Find GC roots" window. * Added progress reporting to Unity when a memory snapshot is being converted from the Unity format to our format.
arturaz
force-pushed
the
refactor-and-fix
branch
from
e01e44e
to
5f59252
Compare
|
Thank you for the time and effort you put into Heap Explorer to improve it and of course for the PR. The changes look really useful, but it will take me a while to go over all of this, because as you wrote, it's a massive change-set. |
|
On a brief inspection, I found two things that no longer work:
Can you fix these issues? |
The old data files contain fields serialized with `-1` offsets. We know these are invalid and should be skipped during reading.
|
Hey there. Fixed the things you requested. |
|
By the way, if you see this pattern: {if (someOption.valueOut(out var someValue)) {
// code goes here.
}}Then the extra curly braces are to prevent the |
|
Thank you for the fixes. I'm most likely going to take a look at it over the course of the weekend. |
…ng read for non-array types. Now instead of having `bool isArray` and `PInt arrayRank` we have one field `Option<PInt> arrayRank` which makes sure we are not capable of representing the illegal state.
|
Trying to open the latest changes from this PR in Unity 2020.3.38f1 (.NET Standard 2.0) shows the following compile errors: Can you fix these compile errors and make sure it runs in Unity 2020.3? |
…ed namespaces yet.
|
Whoops, that fell through :) Fixed! |
|
Thank you for the fixes. I tested your recent changes. Unfortunately there are compile errors in the Heap Explorer tests. When you add Can you fix the compile errors? |
Fix for Unity 2022.2
Tested with Unity 2019.4.40f1.
|
Fixed it. For real this time. Sorry about the previous ones. I checked out the package with Unity 2019.4.40f1 and made sure it compiles and the tests pass.
|
|
Hi! I merged this PR on top of the current head: https://github.com/JonnyOThan/UnityHeapExplorer The referencing field info is fantastic! But I'm sometimes seeing issues where clicking on empty shell objects does not display their fields. There are a few errors from the log as well: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi there.
First of all, let me express how much I am thankful for the work you have put into the HeapExplorer. I had to find out a memory leak recently and it is astonishing how bad the Unity Memory Profiler is at telling the reason of why an object is pinned in the memory. And that's 5 years after its announcement!
HeapExplorer helped me to track down several leaks, but then I noticed that I still had an object lingering (as evidenced both by debug code and Unity Memory Profiler) which your tool did not see. That meant that HeapExplorer has a bug, which I decided to track down.
HeapExplorer seems to have been written pre-modern Unity/C# days, so in process of understanding how it works I changed a lot of code. The changed code uses a bit different coding paradigms, uses newer C# version and is large, which I know is a lot to take in in a pull request. But I do believe it is a change for the better and I hope you will merge this into the main codebase.
If you decide that you don't want that, I would be grateful if you pointed out to the existence of this fork in your main repository
README.mdfor the users.As always, if you have any questions you can ping me at Discord (arturaz#1619) for rapid information exchange. I am more than glad to help understand any of the things I have changed, even though I did my best to document everything in code.
Here's a full list of things that were changed.
Introduced the
Option<A>type to represent values that might not be there.-1) withOptionto indicate the possibility of failure.Replaced possibly-invalid types with
Options. Now if you if have an instance - it's guaranteed to be valid.Replaced magic values (like using positive integers for managed object indexes and negative integers for static object indexes) with proper semantic types to help with code clarity.
Replaced dangerous numeric casts that could under/overflow with typesafe alternatives (
PIntpositive 32bit integer type) that guarantee no such things can happen.Replaced instances of
Listwhich were being used as stacks withStack.Replaced as many fields in objects as possible with readonly ones to indicate that we have no intent of changing them after the object is constructed. This is a part of 'objects should stay valid after construction' philosophy.
Added logging for potentially dangerous operations (like int -> uint).
Replaced primitive loop guards with cycle tracking that tracks seen objects. This eliminates the false positives and errors out sooner if we actually have an error.
Replaced places where things got cancelled silently with emitting warnings / errors. Failures should never be silent.
Fixed algorithm that checks if a type contains reference types: it didn't account for nested value-types which eventually have reference types in them and only looked for reference types in the first level.
Fixed bad lookups in
PackedCoreTypes. Also madePackedCoreTypesto be fully initialized, without any missing entries.Added a warning about the inability to crawl the
[ThreadStatic]variables.Improved search speed by:
OrdinalIgnoreCasecomparisons.Improved documentation by adding new docstrings and converting existing docstrings into proper XMLDocs. Now IDEs and tools can properly render the documentation.
Rewrote a part of code using C# 7.3 (supported since Unity 2019) to enhance readability.
bumped version to 4.1.2
Added capability to copy the root path as text by right-clicking on the root entry in "Find GC roots" window.
Example: