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

[REG-2191] Speedup game action analysis #360

Merged
merged 13 commits into from
Nov 25, 2024
Merged

Conversation

nAmKcAz
Copy link
Collaborator

@nAmKcAz nAmKcAz commented Nov 21, 2024

  • Dramatically shortens the time required to analyze game actions

    • Loads scenes in preview instead of actually modifying the loaded scenes in the editor (this is both for performance and functional to not mess with the user's editor state)
    • Caches type field/property resolutions
    • Reduces 'new' object allocations/gc and prefers re-use
    • Fixes various code style warnings in the analysis file
    • Optimizes the multi loop code analyzer that always ran at least 2 passes to only run >1 passes when necessary
      • Confirmed with Beyond Compare text on a large customer project that before and after results are equivalent
    • Uses multithreading to run resource analysis and code analysis in parallel (also parallelizes parts of code analysis itself)
      • Still provides detailed status/cumulative-progress updates during the multithreaded operations
  • I was honestly hoping to get this down to single digit seconds for ALL games so that we could include it as a compile time trigger automatically. Sadly... that's just not possible for large games as we are limited by Unity's Main Thread access restrictions when doing game resource analysis.

  • Bossroom Before (~13 sec)
    RG_ACTION_ANALYSIS_PERF_OLD

  • Bossroom After (<4 sec)
    RG_ACTION_ANALYSIS_PERF_MultiThread

  • Large Customer Project before (~120 sec)
    RG_ACTION_ANALYSIS_PERF_cust_OLD

  • Large Customer Project after (<40 sec)
    RG_ACTION_ANALYSIS_PERF_cust_MultiThread


Find the pull request instructions here

Every reviewer and the owner of the PR should consider these points in their request (feel free to copy this checklist so you can fill it out yourself in the overall PR comment)

  • The code is extensible and backward compatible
  • New public interfaces are extensible and open to backward compatibility in the future
  • If preparing to remove a field in the future (i.e. this PR removes an argument), the argument stays but is no longer functional, and attaches a deprecation warning. A linear task is also created to track this deletion task.
  • Non-critical or potentially modifiable arguments are optional
  • Breaking changes and the approach to handling them have been verified with the team (in the Linear task, design doc, or PR itself)
  • The code is easy to read
  • Unit tests are added for expected and edge cases
  • Integration tests are added for expected and edge cases
  • Functions and classes are documented
  • Migrations for both up and down operations are completed
  • A documentation PR is created and being reviewed for anything in this PR that requires knowledge to use
  • Implications on other dependent code (i.e. sample games and sample bots) is considered, mentioned, and properly handled
  • Style changes and other non-blocking changes are marked as non-blocking from reviewers

@nAmKcAz nAmKcAz changed the title Speedup game action analysis [REG-2191] Speedup game action analysis Nov 21, 2024
@nAmKcAz nAmKcAz marked this pull request as ready for review November 22, 2024 15:15
@nAmKcAz nAmKcAz requested a review from vontell as a code owner November 22, 2024 15:15
@nAmKcAz nAmKcAz requested a review from abeizer November 22, 2024 15:16
@abeizer
Copy link
Contributor

abeizer commented Nov 22, 2024

I was honestly hoping to get this down to single digit seconds for ALL games

This improvement is huge though! Very impressed by the time save

Copy link
Contributor

@addisonbgross addisonbgross left a comment

Choose a reason for hiding this comment

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

I'm sure this could use at least one more review from someone other than I, to give these changes a thorough check, but overall this is excellent

{
currentExpr = memberExpr.Expression;
} else
}
else
Copy link
Contributor

Choose a reason for hiding this comment

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

most satisfying change of this PR

Edit: I see you change this a few times 👏🏻

var targetAssemblies = new List<Assembly>(GetTargetAssemblies());
for (int i = 0; i < targetAssemblies.Count; ++i)
// some await to make this async on another thread right away
await Task.CompletedTask;
Copy link
Contributor

Choose a reason for hiding this comment

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

would await Task.Yield() have the same effect? Just curious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it doesn't have exactly the same effect, but similar intent

await Task.Yield() will absolutely ensure you get a new synchronization context for the following lines (force them to run async)

await Task.completedTask is quite a bit faster to execute, but does not strictly guarantee a new context.. however, since I spawned an async task, i really just needed something to make that async code 'valid' (have an await)... based on how the code evolved I could even remove 1 or 2 of these and it work the same, but I left them in to ensure that future updates/modifications wouldn't un-expectedly change the threading behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to learn, thanks

@nAmKcAz nAmKcAz merged commit f6ca99a into main Nov 25, 2024
2 checks passed
@nAmKcAz nAmKcAz deleted the zack/speedup-action-analysis branch November 25, 2024 13:23
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.

4 participants