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

Use std::vector instead of Table #12995

Merged
merged 1 commit into from Nov 26, 2021
Merged

Conversation

pcafrica
Copy link
Contributor

@pcafrica pcafrica commented Nov 26, 2021

Follow-up from #12993.

This fix bypasses the memory leak, which by the way still requires further investigation.

closes #12993

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good local solution. Next we need to find out the problem for the AlignedVector.

@peterrum
Copy link
Member

/rebuild

@pcafrica pcafrica marked this pull request as ready for review November 26, 2021 17:05
@peterrum
Copy link
Member

@elauksap What is the reason that this PR is still "draft"?

@pcafrica
Copy link
Contributor Author

@elauksap What is the reason that this PR is still "draft"?

Just removed that label 😉 I was waiting for the tests on my local machine to complete.

@peterrum peterrum merged commit b3851dc into dealii:master Nov 26, 2021
@bangerth
Copy link
Member

I like the solution with the statically typed array. Would this be something we could also apply to other similar classes? If so, want to write a patch?

@pcafrica
Copy link
Contributor Author

pcafrica commented Nov 29, 2021

@bangerth thanks, I'd be glad to help! It's not easy to spot similar cases in the library, but will try my best.

For example, ShapeInfo seems one of those classes to investigate.

@bangerth
Copy link
Member

I was specifically thinking of the other polynomial classes.

@pcafrica
Copy link
Contributor Author

I was specifically thinking of the other polynomial classes.

Ok, I see! You mean to replace, e.g., Tensor<1, dim> by std::array<GradType, dim> and so on like here and in all similar places, right?

@bangerth
Copy link
Member

I think that doesn't work because you don't know how many basis functions you have, and so it really needs to be a dynamic std::vector or Table rather than a fixed size std::array. The beauty of your patch is that it avoids the dynamic allocation of memory by a statically sized object.

I don't know whether there are other similar opportunities in the classes I mentioned. It's possible that there are none, I was just wondering whether there are :-)

@pcafrica
Copy link
Contributor Author

As far as I can see, after some searching and grepping, there is no other polynomial class where this patch can be applied.

@bangerth
Copy link
Member

OK, thanks for checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak with simplex meshes
4 participants