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

[C2A] Fixed linear search of dictionaries #230

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

takahiro0327
Copy link
Collaborator

@takahiro0327 takahiro0327 commented Feb 21, 2024

Fixed a linear search of the dictionary.
Please merge if you like.

I don't normally fix these details, but this time I fixed it based on the following conditions.

  • Shorten several hundred ms when the number of bones is large.
  • The code is simply cleaner.

For one object, this modification reduced the processing time of UpdateAcesoryMoveFromInfoAndReassignBones from 400 ms to 200 ms.
It was probably due to the fact that it was fitted with something with a specially high number of bones.
For other calls, there were three cases of tens of milliseconds and hundreds of cases of less than 1 ms.

The contribution to loading time is approximately 1% for the scenes tested.

before ave: 38.04

Scene loading completed: 38.7[s]
Scene loading completed: 37.5[s]
Scene loading completed: 37.9[s]
Scene loading completed: 38.0[s]
Scene loading completed: 38.1[s]

after ave: 37.62 (-0.42)

Scene loading completed: 37.8[s]
Scene loading completed: 37.6[s]
Scene loading completed: 37.1[s]
Scene loading completed: 38.0[s]
Scene loading completed: 37.6[s]

This should be beneficial as a correction to reduce processing time in worst case situations with many bones.

@takahiro0327
Copy link
Collaborator Author

By the way, this is the end of this type of PR.
Rest assured that I will not be posting any more in succession.

As far as the profiler results are concerned, there is probably nothing that can be improved beyond a few hundred milliseconds.

Copy link
Contributor

@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.

Good catch, no idea why it was done this way.

I wouldn't mind more of this kind of PRs, I bet there's still things that can be sped up somewhat.

@ManlyMarco ManlyMarco merged commit e895974 into IllusionMods:master Feb 21, 2024
@takahiro0327
Copy link
Collaborator Author

Thanks for merge.

No, the only thing at the top of the profiler anymore is the code of the game itself, or minefields like MaterialEditor, or File IO.
It would be delicious if we could find a place where we could cut down on the calls themselves, like OVERLAY.
There is no denying that it is not cost-effective, although there would be something if I were to look for it.
MonoProfilerOutput_2024-02-21_01-10-36.csv

@ManlyMarco
Copy link
Contributor

ManlyMarco commented Feb 21, 2024

Yeah, at this point the only major performance gains would be from optimizing the game code to not run as often (e.g. code that applies bone morphs in LateUpdate); or from running things in parallel threads like I did with sideloader (which is not possible if the code needs to access any Unity APIs).

Still, you did a great job at optimizing it, thank you.

@takahiro0327
Copy link
Collaborator Author

Thanks.

Did you do thread parallelization in Sideloader? That's great.

Hmmm... I wonder if there's any purely slow code in the plugin that doesn't touch UnityAPI...
That's not loading, but I think DynamicBone's Update would benefit from going multicore.
Oh, by bone morph do you mean DynamicBone?

@takahiro0327 takahiro0327 deleted the c2a-mini-refactor branch February 21, 2024 13:37
@ManlyMarco
Copy link
Contributor

ManlyMarco commented Feb 21, 2024

Oh, by bone morph do you mean DynamicBone?

Both DynamicBones and applying bone scales and morph values from custom data. This is most of what is done in update methods.

Hmmm... I wonder if there's any purely slow code in the plugin that doesn't touch UnityAPI...

It's pretty much only limited to loading things from files, unfortunately.

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.

2 participants