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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic Improvements #140

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

fponta
Copy link
Contributor

@fponta fponta commented Jan 5, 2024

This PR is a mix of small things.

  1. The most important part is the usage of sorted containers to improve the performance of the output modules:
    whenever the boundary conditions are numerous, the retrieval of strain and stresses on an element becomes the most time consuming part, not because the computations are hard per se, but because most of the time is spent looping through all the BCs looking for which of them are applied to such element. This suggests the best practice would be to merge together equivalent boundary conditions by merging the sets. At that point, in the case of element BCs, the bottleneck would be the O(N) lookup on the elements IntArray. This fix uses the sorted IntArray inside the Set, reducing the complexity to a one shot O(NlogN) and a O(logN) for each time the lookup is performed.
    Note: since these structures are meant for lookup, in terms of performance an std::unordered_set (O(1)) would be better, but the memory footprint may be too much when the sets are a lot? 馃

  2. The initialization of the staggered solver has prob3 as an optional parameter. To retrieve that, the reference to the third element in the array is still passed around. This harmless in the Release build, the Debug build however performs out of bound checks and complains.

  3. The rest is unused variables and macros

@fponta fponta marked this pull request as ready for review January 7, 2024 16:13
@bpatzak
Copy link
Member

bpatzak commented May 22, 2024

I understand an issue, and yes, there is a room for a great optimization.
Just some notes here to discuss first:

  • perhaps we can consider representing sets using std::bitset, that would provide constant time for testing if element is in the set, and can implement iterator over set elements. Bitsets are extremely memory efficient. The iterator, however will be more expensive, compared to current implementation.
  • Perhaps we can internally support both representations: keep the present implementation for smaller sets (Number of elements less than certain threshold) and use bitsets for larger sets.

@fponta
Copy link
Contributor Author

fponta commented May 30, 2024

There are several possibilities indeed, even a new derived set class optimized for some use cases. For the moment I just tried to reuse what's already in the code base to give it a quick boost.

It may take some time to evaluate pros and cons, make benchmarks, etc. In any case the effort needs to be worthwhile especially on your side...probably it isn't much of a priority? Up to you.

p.s. Just as a heads-up: I have mid-high range laptop, yet building everything (with parallel compilation enabled) takes about 20 minutes 馃槄
In the future I'd like to create other PRs that will gradually remove unused included headers and remove duplicated macros and symbols. Hopefully in the end I will probably also be able to leverage jumbo builds.

@bpatzak
Copy link
Member

bpatzak commented Jun 7, 2024

I just committed an attempt to address this to devel branch. At the element level it is avoiding the testing of element presence in set completely and there is only the loop over relevant BCs.
Can you try to evaluate it on your examples?

You compile on Windows, right? On my linux laptop, the oofem compilation (debug mode, python bindings, using 10 cores) takes just 5 minutes,

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.

None yet

2 participants