Do not pass ownership of the QueryStack in Runtime::block_on_or_unwind
#626
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #626 will not alter performanceComparing Summary
|
Veykril
force-pushed
the
veykril/push-koortqxmzkop
branch
4 times, most recently
from
c3873e9 to
d1a944e
Compare
Veykril
force-pushed
the
veykril/push-koortqxmzkop
branch
from
d1a944e to
b3e745b
Compare
|
@carljm you should probably take a look at this to prevent that it makes changes that are incompatible with your new fixpoint cycle handling. |
|
I'll have to resolve some merge conflicts, but it looks fine. The cycle-handling code changed here totally disappears in the fixpoint-iteration branch, and the query-stack API changes shouldn't make any difference to that branch. |
…wind` This commit changes `Edge` such that it no longer takes direct ownership of the query stack and instead keeps a lifetime erased mutable slice, an exclusive borrow and as such the ownership model does not change. The caller now does have to uphold the safety invariant that the query stack borrow is life for the entire computation which is trivially achievable as the caller will block until the computation as done.
Veykril
force-pushed
the
veykril/push-koortqxmzkop
branch
from
b3e745b to
c57aa8b
Compare
|
(To be clear, I haven't carefully reviewed these changes for correctness, and won't have time to do that. But assuming they are reviewed and are good for current Salsa, I'm not worried about their impact on fixpoint iteration. And I don't think other changes should be delayed to wait for fixpoint iteration, since that branch is not ready yet. Resolving merge conflicts isn't a big deal.) |
src/runtime/dependency_graph.rs
Outdated
| pub(super) struct Edge { | ||
| pub(super) blocked_on_id: ThreadId, | ||
| pub(super) blocked_on_key: DatabaseKeyIndex, | ||
| stack: SendNonNull<[ActiveQuery]>, |
There was a problem hiding this comment.
Contemplating whether to use a raw pointer here, or just transmute the lifetimes and store a &'static mut [ActiveQuery] instead. The latter probably simplifies a bit
|
As discussed, decision on this should either way wait until the fixpoint PR is finished. |
Builds on top of #624, opened separately as this is this is separate concern that is more likely to be rejected.
This is more of a micro optimization as it reduces the size of
Edgeby ausize(and it removes onemem::takeon aVec)Motivation for this change is to reduce the moving of the query stack
Vec, that removes a bunch of potential memcopies (at the expense of some more unsafe)