Fix full chain algorithm deallocation order #664
Conversation
stephenswat
added
bug
stephenswat
force-pushed
the
fix/wrong_destructors
branch
from
23edcba
to
83a3099
Compare
There was a problem hiding this comment.
I would technically do it a little differently. 🤔
I'm a big fan of explicit destructors for any non-trivial class. Since as long as you declare the destructor, it's code is only generated once. But with no declaration every client has to generate the same code over and over again.
So I would either make the existing functions empty, or possibly declare them (in the cpp files) with = default. (That's a valid syntax... right?)
| full_chain_algorithm::~full_chain_algorithm() { | ||
|
|
||
| // We need to ensure that the caching memory resource would be deleted | ||
| // before the device memory resource that it is based on. | ||
| m_cached_device_mr.reset(); | ||
| } |
There was a problem hiding this comment.
You didn't touch the header file...
This is a nice idea, but it's one of those things that turns out a bit more hairy than you initially think. That is to say, there are some pretty nice upsides to implicitly-declared destructors, which stem from the amount of information the compiler has about them. For one, if you explicitly specify a destructor you lose the trivial destructibility of the class, as the compiler cannot statically determine if the destructor is trivial if it is user-defined and not defaulted at first declaration. Of course these classes are not trivially destructible anyway, but I'd say it's almost always better to let the compiler handle these things. More interestingly, implicitly declared destructors have automated throw-specifiers, while user-provided ones do not. If you want to properly throw-specify an explicit destructor, you'd have to check every single one of your members, while the compiler can actually handle that by itself. So, you lose a few useful functional properties about your class and you add additional lines of code, while all you gain is some compile-time speed-up. That would be worth it if the class was used very frequently but these destructors are instantiated twice; I'm not sure the impact on compile time is even worth talking about. Long story short, I'm really not sure if this is worth the hassle; the upsides on both sides are so incredibly minor. Of course, if you insist I'll change it. 😉 |
|
I became sensitive to these issues with VecMem, and symbol visibility. If you require clients to generate their own destructors, they have to see all the symbols of the class as well. Which we don't always want to do. In those cases explicitly declared default constructors helped us out.
Eventually I'll want the project's "algorithms" to expose about themselves as little as possible. So I'll very much want to have explicit destructors on pretty much everything. As I started, none of this applies to "simple" classes of course. Just to things that construct/destruct types that we may or may not want the user to be aware of. |
stephenswat
force-pushed
the
fix/wrong_destructors
branch
from
83a3099
to
30caec2
Compare
Since acts-project#595 was merged, some of the throughput examples started to fail. After some investigation, it turned out that this was not actually a mistake in acts-project#595, but rather a long-standing bug in the full chain algorithms. The situation was such that the full chain algorithms had custom destructors which destroyed the caching memory resources, which are pointers that need to be destroyed before the underlying memory resource they use. This creates a problem, namely that the cached memory resource is destroyed before _any other_ members of the class, including any long-standing memory allocations. When those allocations are then destroyed, the memory resource no longer exists and the program segfaults. Thankfully, the fix for this was very easy as the aforementioned destructors are not necessary at all, as the C++ standard guarantees that members are destroyed in reverse initialization order, and since our full chain algorithms always (correctly) specify the caching memory resource _after_ the base memory resource, the default destructors are more than sufficient. In order to fix the segmentation fault, this commit simply removes the offending destructors.
|
Sure mate, whatever floats your boat. 👍 |
Since #595 was merged, some of the throughput examples started to fail. After some investigation, it turned out that this was not actually a mistake in #595, but rather a long-standing bug in the full chain algorithms.
The situation was such that the full chain algorithms had custom destructors which destroyed the caching memory resources, which are pointers that need to be destroyed before the underlying memory resource they use. This creates a problem, namely that the cached memory resource is destroyed before any other members of the class, including any long-standing memory allocations. When those allocations are then destroyed, the memory resource no longer exists and the program segfaults.
Thankfully, the fix for this was very easy as the aforementioned destructors are not necessary at all, as the C++ standard guarantees that members are destroyed in reverse initialization order, and since our full chain algorithms always (correctly) specify the caching memory resource after the base memory resource, the default destructors are more than sufficient.
In order to fix the segmentation fault, this commit simply removes the offending destructors.