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

Increase performance of irr::core::array #12251

Closed
paradust7 opened this issue Apr 30, 2022 · 4 comments
Closed

Increase performance of irr::core::array #12251

paradust7 opened this issue Apr 30, 2022 · 4 comments
Labels
Concept approved Approved by a core dev: PRs welcomed! Performance Request / Suggestion The issue makes a suggestion for something that should be done but isn't a new feature @ Startup / Config / Util

Comments

@paradust7
Copy link
Contributor

paradust7 commented Apr 30, 2022

Problem

Emscripten build profiling shows that at least 15% of CPU time is spent in irr::core::array::insert during some periods of stutter. In this particular example, the most time was spent in PartialMeshBuffer::beforeDraw, which does an element-by-element push_back from a std::vector to an irr::core::array.

Screenshot from 2022-04-29 23-18-41

(this chart comes from samples of the call stack over a 7 second period)

There are many optimizations that are not done in irrArray, most importantly, optimizing bulk copies (using e.g. memcpy).

Solutions

Proposed solution:

Modify irr::core::array to use std::vector internally, while keeping the same external interface.

The TAlloc/irrAllocator template parameter appears to never be used, so can be removed. setAllocStrategy is also never used, so can be removed. The only other feature that cannot be replicated is set_pointer, which alters the internal pointers of the array. This is only used in one place (CGUITTFont.cpp), so it should be possible to refactor it manually. All other uses of irr::core::array would remain the same.

Finally, it is now possible to add additional methods to support bulk copying/moving/swaping of data between std::vector and irr::core::array. This will make it possible to have PartialMeshBuffer::beforeDraw do a trivial move, instead of an allocation and copy.

@paradust7 paradust7 added the Feature request Issues that request the addition or enhancement of a feature label Apr 30, 2022
@Zughy Zughy added Performance Request / Suggestion The issue makes a suggestion for something that should be done but isn't a new feature and removed Feature request Issues that request the addition or enhancement of a feature labels Apr 30, 2022
@sfan5
Copy link
Member

sfan5 commented Apr 30, 2022

All of the irrlicht containers should be gotten rid of eventually, but replacing their internal implementation with stdlib types sounds like a good first step.

The only other feature that cannot be replicated is set_pointer, which alters the internal pointers of the array. This is only used in one place (CGUITTFont.cpp), so it should be possible to refactor it manually.

That's not so simple unfortunately. CGUITTFont wants to call drawing methods that only take an irr::core::array. But it wants to do this without copying the data, which is std::vector can't support. So the only alternative is to change the API.

@sfan5 sfan5 added the Concept approved Approved by a core dev: PRs welcomed! label Apr 30, 2022
@paradust7
Copy link
Contributor Author

paradust7 commented May 1, 2022

I have an implementation of this which seems to be working, but I still need to do a lot of testing and verification. So it is not quite ready for review.

minetest/irrlicht@8f2c0f2

65fe97e

@sfan5
Copy link
Member

sfan5 commented May 1, 2022

		// std::vector<bool> can't be used here because it doesn't
		// allow taking a bool& reference to individual elements.

If you need that you could use std::vector<u8> instead of writing a custom implementation.
(would require changing the type but that's not a problem)

@paradust7
Copy link
Contributor Author

Making this backwards compatible would be painful. All changes in both repos would have to be behind ifdef.

Because each repo's PR is separate and they both need to be applied to work together, the workflow tests can't test anything except backwards compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Performance Request / Suggestion The issue makes a suggestion for something that should be done but isn't a new feature @ Startup / Config / Util
Projects
None yet
Development

No branches or pull requests

3 participants