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

Aggregate button added #65

Merged
merged 13 commits into from
Feb 3, 2024

Conversation

takahiro0327
Copy link
Contributor

Aggregate button added. It aggregates ticks and gcbytes of the same function.
Please merge if you like.

aggregation

It was clear that DynamicBone was the slowest, but we wanted to be sure. I also wanted to make sure that there were not a large number of instances at the bottom of the list that would cause slowness.

@takahiro0327 takahiro0327 marked this pull request as draft January 31, 2024 09:29
@takahiro0327
Copy link
Contributor Author

After reviewing it, the key brackets were lame, so I changed them to columns.
image

@takahiro0327 takahiro0327 marked this pull request as ready for review January 31, 2024 10:42
@ManlyMarco
Copy link
Owner

How does aggregation affect the order column ? Is the order counted from the 1st instance or is it random?

@takahiro0327
Copy link
Contributor Author

Kind of random.
To be precise, the order in which they are added to _data is the oldest.

The first value in the sequence of GroupBy return values is used. (group.First())
This is the first value found in a sequence of _data.Values in ProfilerInfo belonging to a group.

The order of Dictionary.Values used in KKS is the order in which they were added.
So it will return the numeric value of the oldest instance in the group.
If the scene was just loaded, you might expect this to be close to the smallest value in the group.

We can still read the rough order, but should we make it so that the smallest value in a group is displayed?


This is unrelated, but added re-sorting when changing options.

@takahiro0327
Copy link
Contributor Author

takahiro0327 commented Feb 1, 2024

Fixed a problem that the data is not aggregated when _needResort flag is OFF.
Fixed to make order values more stable.

@ManlyMarco
Copy link
Owner

I think the smallest value should be used for the order.

@takahiro0327
Copy link
Contributor Author

Is it this way?
What difference does it make?

image

@ManlyMarco
Copy link
Owner

I just took a look at the source and the current Max implementation should be fine. I'll test it later and merge if all is good.

@takahiro0327 takahiro0327 marked this pull request as draft February 1, 2024 14:43
@takahiro0327
Copy link
Contributor Author

Sorry if you have already tested it.
I noticed a bug and will fix it.

@takahiro0327
Copy link
Contributor Author

Bug fixed.
Past values were included in the aggregate.

@takahiro0327 takahiro0327 marked this pull request as ready for review February 1, 2024 15:06
@ManlyMarco
Copy link
Owner

ManlyMarco commented Feb 1, 2024

Tested the latest commit. There appears to be an issue where the list stops updating after enabling and then disabling Aggregation. It sometimes resumes updating after closing and reopening the profiler window, but then the list appears to be corrupted. Unhooking and then re-hooking fixes the issue.

Normal
image

After enabling aggregation
image

After disabling aggregation (list no longer updates)
image

After closing and reopening the profiler window (list should be exactly the same as in step 1 but the order values are different and sometimes it doesn't update)
image

@@ -307,10 +321,25 @@ private IEnumerator FrameEndCo()
}
}

if (_needResort)
if (_needResort || _aggregation)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be a part of the issue, it shouldn't be resorting on every frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required to update aggregated values.
Aggregated values are not updated by Postfix.

@takahiro0327
Copy link
Contributor Author

Thanks for the review.
Fixed. How about this.

@ManlyMarco
Copy link
Owner

ManlyMarco commented Feb 2, 2024

It updates correctly now. The only issue left that I can see is the "Ran" column shows the right toggle as off incorrectly when it should be on.

It might be best to remove the right toggle in Aggregate mode since some methods might run and others might not. Maybe replace the column with "any ran". Alternatively 3-state checkboxes could be used I suppose, but I'm not sure if IMGUI supports that.

@takahiro0327
Copy link
Contributor Author

Sorry, I didn't think that far ahead.

Remove right toggle when in aggregate mode.

Separate aggregate from executed and non-executed. The toggle on the left should now be accurate.

Is the SinceLastRun type of byte an intentional specification?
Sometimes there were moments when it overflowed and things that were not working seemed to be working.
I changed the type to uint for now.

Copy link
Owner

@ManlyMarco ManlyMarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good now, thank you for the PR!

@ManlyMarco ManlyMarco merged commit 4dccc9e into ManlyMarco:master Feb 3, 2024
@takahiro0327 takahiro0327 deleted the aggregate_button_added branch February 3, 2024 15:35
@takahiro0327
Copy link
Contributor Author

Thanks too for the review and the useful tool!
This profiler helped me fix my code which was slow.

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

2 participants