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.

Sign up for GitHub

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

Jump to bottom

[v2.0.0] Add benchmarks to allow for proper profiling #12

Open
lmichaelis opened this issue Oct 3, 2022 · 5 comments
Open

[v2.0.0] Add benchmarks to allow for proper profiling #12

lmichaelis opened this issue Oct 3, 2022 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone
v2.0.0

Comments

@lmichaelis
Copy link
Member

There is probably a lot of performance left on the table at the moment since it is not really possible to profile most parts of phoenix. Especially the script VM is critical to performance in Gothic re-implementations like OpenGothic.

Writing proper benchmarks will enable easier profiling of phoenix.
@lmichaelis lmichaelis added this to the v1.1.0 milestone Oct 3, 2022
@lmichaelis lmichaelis added the enhancement New feature or request label Oct 3, 2022
@lmichaelis lmichaelis added the good first issue Good for newcomers label Oct 25, 2022
@Try
Copy link
Contributor

Try commented Nov 6, 2022 •
edited
Loading

Note about load-time performance:
изображение

Animation::Sequence::Sequence in OpenGothic calls for Resources::vdfsIndex().find_entry twice, making total of 18% of load-time.

One fix is easy - use one call, instead of 2 - that is on me. Yet find_entry probably can be improved.

@lmichaelis
Copy link
Member Author

Hm that's an interesting profile. I did one about a week ago and it actually showed that vdf_file::merge takes a long time to run. That is probably related to me using an unsorted Container for vdf_entry::_m_children. I'm looking into a better solution potentially using a set or a list.

@lmichaelis
Copy link
Member Author

As expected, using a std::set with a custom, case-insensitive comparator, I see about 80x performance improvement in find_entry. That's exceptional! I'll have to find a way around std::set's const lookups but it seems like the only right way of doing things is to deprecate the non-const find_entry APIs. They could be re-added in the future because I'm planning a rework of the VDFS API anyways.

lmichaelis added a commit that referenced this issue Nov 7, 2022
@lmichaelis
[#12] vdfs: improve VDF lookup times drastically
604b61b 
A basic benchmark shows that using a sorted `std::set` instead
of a raw, unsorted `std::vector` improves entry lookup times
as well as VDF merge times by up to 90x.

While this commit does introduce a bunch of `const_cast`s, they
are mostly guarded by deprecation warnings and unless
`vdf_entry::name` is touched, they should still be safe.
@lmichaelis
Copy link
Member Author

@Try I'm keeping a bleeding-edge version of OpenGothic with the newest patches at lmichaelis/OpenGothic in the phoenix-bleeding branch. Feel free to pull/merge from there if you like as well :)

@Try
Copy link
Contributor

Try commented Nov 7, 2022

Updated OpenGothic - very nice improvement of load-time performance, thanks!

@lmichaelis lmichaelis modified the milestones: v1.1.0, v2.0.0 Jan 29, 2023
@lmichaelis lmichaelis changed the title [v1.1.0] Add benchmarks to allow for proper profiling [v2.0.0] Add benchmarks to allow for proper profiling Jan 29, 2023
@lmichaelis lmichaelis added this to ZenKit May 4, 2023
@lmichaelis lmichaelis moved this to todo in ZenKit May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
ZenKit
Status: todo
Milestone
v2.0.0
Development

No branches or pull requests
2 participants
@Try @lmichaelis