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

Improve perf #1

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

Conversation

blu-base
Copy link

@blu-base blu-base commented Jul 6, 2021

Hi Truong,

I saw your post on cfdonline.com. As the other commenters pointed out, there are some inefficiencies in your code.
Although i am not very experienced in software development either, i applied my current experience with this pull request.

Most changes are regarding changing copying objects to copying their reference only. Moreover, I applied std algorithms where I think they apply. This reduced the call graph by about a magnitude, and reduced the run time by about a half (1.2 seconds to 0.66 seconds on my machine). The serial parts such as writing could be accelerated by avoiding std::endl since this always contains a std::flush. I replaced it with '\n' to keep it in memory as long as possible. Though there is the disadvantage that it leads to higher memory consumption. For very large files, there might be more intermittent flushes necessary.

The slowest part is now the FvmMesh2D::detectNearestNeighbor() method. The current changes to that method are worth about half of the performance gains mentioned above. I believe, this state seems to be not optimal. I didn't attempt to think this part through though...

Additum:
I added a cmake file for my convenience. Since I did not know how you label the executable, I set the placeholder "gmshToX", lacking any good ideas, here =)

Best,
Sebastian

@truong-pham-dang
Copy link
Member

Hi Sebastian,

Sorry for late response. Currently your pull request has conflicts. Can you resolve that? I'm very happy to merge your pull request right away.

Best regards,
Truong

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