Repository navigation
TimeEvolver-owned allocator - #484
Conversation
|
Need professional opinions since I'm not familiar with TensorOperations.jl infrastructure. |
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Overall this looks perfect, I left a small comment on the use of finally, although I do think that in principle you might be right there, it might just be overkill.
The one thing I would recommend to try and keep is that indeed, reusing the buffer helps, but I'd try and avoid as much as possible having it leak out of the public-interface functions, and have this as an automatic behind-the-scenes approach as much as possible. That is in line with this PR I think, since I'd say manually constructing the evolver is expert-use, but I just wanted to mention that here again, as I do think it is quite important, because it is very easy to mis-use
| (iter == it.nstep) && return nothing | ||
| psi, env, ϵ = su_iter(state.psi, it.circuit, it.alg, state.env) | ||
| cp = allocator_checkpoint!(it.allocator) | ||
| psi, env, ϵ = try |
There was a problem hiding this comment.
I see why the try - finally is here, but I think in principle this is not really necessary/consistent with the rest.
We more or less made the decision that indeed, if there would be an error, the internal state of the allocators might be corrupted and you might end up with memory leaks. I'm not saying this is the best decision, and in principle we might want to avoid this, but since this is already the case in the TensorOperations implementation itself, the finally here would only help for a very small amount of cases, so probably it's not really worth it.
Specifically here, I think the allocator will go out of scope as soon as the time evolver does, so that would already fix the bumper, while actually using a manual allocator would leak memory anyways, even with the finally clause.
There was a problem hiding this comment.
Dropped the checkpoint and the try-finally block.
Previous work on simple update (#459, #483) introduced reusable allocators to handle contractions during a single
dtstep. This PR extends the lifespan of the allocator by creating it with TimeEvolver, so it can be reused across alldtsteps.The additional benefit for simple update is quite small (less than 5% allocation reduction), but can be much more helpful for NTU #144.