Skip to content
This repository was archived by the owner on Feb 19, 2026. It is now read-only.

Make sure to revert all partially established but eventually conflicting borrows. - #25

Merged
adamreichold merged 1 commit into
mainfrom
consistent-borrows
Jan 3, 2022
Merged

Make sure to revert all partially established but eventually conflicting borrows.#25
adamreichold merged 1 commit into
mainfrom
consistent-borrows

Conversation

@adamreichold

@adamreichold adamreichold commented Dec 26, 2021

Copy link
Copy Markdown
Owner

This does add small amount of overhead to Query::borrow and it was actually a bit difficult to write a test case which would trigger this. (Just querying (&mut T, &mut T) is not sufficient as the fields of a tuple are put on the stack and dropped if one of them panics, i.e. this can only happen if the borrow is conflicting only on some but not all archetypes.)

However, it is a correctness question w.r.t. unwind safety even though one cannot circumvent memory safety by exploiting this, just leave behind dead borrows (which are also possible via e.g. forget(query.borrow(world))).

Finally, since we always use panic="abort" in our simulations, this does not really affect us at all and hence I am somewhat torn between making it generally correct and avoiding the complexity and overhead. It even seems that most users of hecs do not care (or at least vocally complain) about the lack of this property either, c.f. Ralith/hecs#209.

@adamreichold adamreichold self-assigned this Dec 26, 2021
@adamreichold

Copy link
Copy Markdown
Owner Author

This does add small amount of overhead to Query::borrow

Well actually, this might be stretching it: I cannot really reliably measure any difference in the get_component_* benchmarks which have the highest borrowing overhead and e.g. showed a small but possibly significant difference in #24. So this then basically boils down to the additional complexity in Query::borrow...

@adamreichold adamreichold changed the title RFC: Make sure to revert all partially established but eventually conflicting borrows. Make sure to revert all partially established but eventually conflicting borrows. Dec 26, 2021
@adamreichold

Copy link
Copy Markdown
Owner Author

Actually, I found a variant that has basically additional complexity whatsoever as QueryRef will already do what we want on Drop, hence we just need to wrap the various references within it before starting to borrow components. So I think this is actually something that we should definitely do.

@adamreichold
adamreichold merged commit e9086af into main Jan 3, 2022
@adamreichold
adamreichold deleted the consistent-borrows branch January 3, 2022 07:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant