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

Fix: performance analytics spikes #2569

Merged
merged 10 commits into from
Oct 24, 2024
Merged

Conversation

popuz
Copy link
Collaborator

@popuz popuz commented Oct 23, 2024

What does this PR change?

Fix #2534

  • disabled analytics for JsHeap (until we find the solution for it)
  • change approach to Sort() and calculation of FrameTime samples, instead of one frame to do it in Update
  • re-arranged hiccups calculation
  • change string building for samples from float to int (as in old client)

Sort() optimization:

  1. Basic idea of Sort() optimization comes from quite simple idea that Sorting already sorted array is much much more efficient than doing it on unsorted one.
  2. Also, in this case we basically distribute sorting through the frames. Imagine bubble sort algorithm, it take one element and then bubble it up and do it for all N elements for the array. In our case, if it would be bubble sort, we would do it only for one element per frame. So at least N operations less. With the binary search it could be a bit different, but basic optimization idea still the same.
  3. Further more, if needed now there are some heuristics and optimizations possible to apply, because of sorting ulong elements and also taking into consideration that new frame should be placed potentially near to the previous frame. I discarded that logic, because profiling showed that current version performs fast. I left more readable version. But we can do it in future if we feel the need.
  4. Now CPU load of the sorting is actually scaling with the performance. Previously, we always were sorting 1000 elements, but now it depends. For higher FPS you sort more for lower FPS less. Distribution of it is like that
    11 ms (90 fps) - 1364 frames
    20 ms (50 fps) - 750 frames
    40 ms (25 fps) - 375 frames

How to test the changes?

Just a smoke test

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

@popuz popuz self-assigned this Oct 23, 2024
@popuz popuz marked this pull request as draft October 23, 2024 14:10
@popuz popuz added the force-build Used to trigger a build on draft PR label Oct 23, 2024
@popuz popuz removed the force-build Used to trigger a build on draft PR label Oct 23, 2024
@popuz popuz marked this pull request as ready for review October 23, 2024 17:32
@github-actions github-actions bot requested a review from NickKhalow October 23, 2024 17:32
@Ludmilafantaniella Ludmilafantaniella self-requested a review October 23, 2024 18:45
Copy link

@Ludmilafantaniella Ludmilafantaniella left a comment

Choose a reason for hiding this comment

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

🟢 Fix verified on Windows and Mac. The explorer run as a charm, no new issues were found during the smoke testing:

  • Social interaction (Passport - chat)
  • Multiplyer (wearables and emotes)
  • Backpack functiuonality
  • World: metadynelabs
  • Minigames: whack frog, color pop
  • Side bar (all shortcuts)

@dalkia dalkia merged commit a9e1570 into dev Oct 24, 2024
5 checks passed
@dalkia dalkia deleted the fix/performance-analytics-spikes branch October 24, 2024 01:13
@popuz popuz changed the title Fix/performance analytics spikes Fix: performance analytics spikes Oct 24, 2024
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.

Performance Analytics System provoking hiccups
3 participants