legion
legion copied to clipboard
amethyst
Per-component borrow checking
Currently, runtime borrow checking is performed in order to allow the user to access two different components at the same time (which rust's static borrow checking would not allow), but disallow the same component from being incorrecttly borrowed multiple times.
The runtime state of a component's borrow is stored as an atomic integer along with each component's slice in each chunk. This is efficient and allows us to include borrow checking in release builds (although this is currently not the case) without significant performance penalty.
However, it is overly strict. It will not allow two components of the same type to be accessed at the same time when they each belong to different entities - even though this should be safe. This makes random component access very difficult in many situations.
We could relax these borrowing restrictions by tracking borrow state for every component, rather than each component slice. However, it is unlikely that this would provide acceptable performance, so we almost certainly would have to disable runtime borrow checking in release builds (perhaps with an opt-in).
However, it is unlikely that this would provide acceptable performance, so we almost certainly would have to disable runtime borrow checking in release builds (perhaps with an opt-in).
Would this not potentially cause undefined behavior in release mode, though? Even if users check that their borrows are correct in debug mode, untested edge cases could cause illegal borrow patterns, leading to UB in safe code. This might be tolerable, but there should definitely be a warning of such cases in the docs (and, technically, any component access functions should be marked unsafe, but that's not very ergonomic.)
Yes, that is expressly insta-UB. It sounds like this is in fact already a present problem:
This is efficient and allows us to include borrow checking in release builds (although this is currently not the case)
The implementation is therefore unsound, and the proposed change would further entrench that unsoundness.
I have created an issue for this: #21
I was undecided on an opt-out vs opt-in for release mode checks. I do not think an opt-out of these checks by itself is an issue. Such an opt-out serves much the same function of requiring the user to aknowledge their responsibility as the unsafe keyword does. However, these APIs do indeed need to be marked as unsafe regardless, and I do agree that the library should not allow the user to write code that breaks borrowing rules without the user's responsibilities being clearly communicated (ideally via unsafe, or very clear documentation if there is no single entrypoint responsible for the potential issue).
The safety checks proposed in this issue will require some way to disable them for performance reasons. Without that, either the borrow checking remains too loosely defined for these APIs to be truely functional, or the library as a while becomes too slow to be usable. If it is ever a decision between being able to solve a problem at the risk of the user being able to write code with certain classes of bugs, or not being able to solve the problem at all, I will choose the former. However, I cannot think of a situation where this cannot be clearly communicated; that is the purpose of unsafe and it is applicable here. The argument for ergonomics is much weaker with the introduction of systems (which ensure safety via the scheduler) and the query random access APIs proposed in #19 (~~which can perform cheap runtime checks~~ actually this is not true - but I think marking these APIs as unsafe is justified).
I do not think an opt-out of these checks by itself is an issue.
I think this is sound so long as the opt-out is itself marked unsafe. Of course, care should be taken to ensure that most users do not need to write unsafe code to make effective use of the library, if at all possible.
If it is ever a decision between being able to solve a problem at the risk of the user being able to write code with certain classes of bugs, or not being able to solve the problem at all, I will choose the former.
To be clear, undefined behavior is not "certain classes of bugs," it's permission for the compiler and run-time environment to do literally anything, including revealing private data or remote code execution. We can't always wall it off completely, but in Rust we can at least make sure it doesn't come out of nowhere.
through the use of cfg-if and a private unsafe function that gets exposed as either a safe or unsafe public function, it's fairly easy to setup that release mode makes things become unsafe.
also please don't actually do any of this, it's a very bad look for the future core of Rust's only major game engine project to have even potential soundness issues.
A flag like that would either introduce compile errors that only exist in one build target (essentially forcing you to aknowledge the unsafe in both cases anyway), or it would cause people to disable runtime checks in debug builds (which I don't think is ever justified).
By marking these functions as unsafe in either case, there are no soundness issues beyond what you might find in any unsafe Rust code, and they are unsafe due to the nessesity of their nature. If anything, the fact that the library can perform runtime sanity checks for this unsafe code and provide stack traces pointing at the origin of the problem is a significant advantage over most unsafe code.
How about instead of changing the current implementation, allow the user to get a "ComponentStorage" wrapper that allows for borrowing the entire storage instead of a single component, and let that storage wrapper expose the decision of unsafety vs safe ref-counting per entity?
