[wasmparser] add atomic validator feature#2429
[wasmparser] add atomic validator feature#2429keithw wants to merge 2 commits intobytecodealliance:mainfrom
Conversation
|
If performance is ok to compromise on a bit here, the |
|
Yeah, we are validating basically on every keystroke, so having to duplicate the operand/control/init stacks on every operator (valid or not) is probably too expensive for us. |
|
Would you be up for doing some quick measurements with the scale of wasm files you're expecting to see what it's like? I'd naively expect keystrokes to be dozens-of-milliseconds apart which is probably more than enough time to clone the validator and such, so my hunch is that it's probably not that noticable but if you're dealing with The reason I ask is that this is pretty invasive in the validator in terms of hooking into the critical points of dealing with all the internal validator state. It's not unreasonable per-se and I'd agree that a compile-time feature is the way to go here, but even with that this is something that may not be that easy to maintain over time. Given that I'd like to try to push on the clone-ability of the validator (which in contrast should be pretty easy to maintain) and see if that works. If it doesn't work though it doesn't work, and this seems reasonable enough. |
FuncValidator::atomic_op validates an operator, leaving the validator unchanged if the operator is invalid.
|
Sure, here's a (very) rough measurement... Let's say the largest file we'd plan to edit is around 100,000 lines. (The clang.wasm that used to be a w2c2 example is >10M lines.) The worst case is probably something like "a single function with 100,000 lines of
(Edit to add: These numbers are for a release build running on native Linux x86-64; the real application is going to be running in Wasm in a browser which I think also makes us a little more concerned about possible performance gotchas.) (Edit 2: made the numbers clearer and made the atomic benchmark actually use the new atomic_op function.) |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Hm well yes to a certain extent I understand that cloning has quadratic behavior and that means it's quite feasible to construct a case that showcase the quadratic behavior and behaves poorly. I was instead curious about more real-world or examples-in-practice you might run into, for example do you expect that you'd be working with multi-thousand opcode functions?
Nevertheless I'll elaborate some more on my concerns about the code organization here as well. I've left a review about various parts below. They're all tractable to fix in my opinion without too too much refactoring, but this is along the lines of what I was trying to avoid by having a third system of a sort to track pushes/pops/etc to keep in sync with everything else.
crates/wasmparser/Cargo.toml
Outdated
| # A feature that allows validating an operator atomically, leaving the validator | ||
| # unchanged if the operator is invalid. | ||
| atomic = [] |
There was a problem hiding this comment.
I might bikeshed the naming here a bit since "atomic" sort of sounds like a wasm proposal along the lines of "simd". Maybe rollback or try-op or something like that?
| // In a debug build, verify that `rollback` successfully returns the | ||
| // validator to its previous state after each (valid or invalid) operator. | ||
| #[cfg(all(debug_assertions, feature = "atomic"))] | ||
| { | ||
| let snapshot = self.validator.clone(); | ||
| let op = reader.peek_operator(&self.visitor(reader.original_position()))?; | ||
| self.validator.begin_atomic_op(); | ||
| let _ = self.op(reader.original_position(), &op); | ||
| self.validator.rollback(); | ||
| self.validator.pop_push_log.clear(); | ||
| assert!(self.validator == snapshot); | ||
| } |
There was a problem hiding this comment.
I'm a little worried about putting this here due to the quadradic behavior of clone/==. For the same reasons your tests are showing it's quite slow, this would drastically slow down development/testing when this feature is enabled.
That being said it's also a really good test to have. One option perhaps might be something like a custom #[cfg(foo)] specified in CI but otherwise not tested anywhere. That way CI would test this, where it's presumably not a massive slowdown, but external consumers wouldn't test it.
There was a problem hiding this comment.
Happy to do that if you want -- do you want a custom cfg or is a custom feature okay? (I may need help on best practices for the former.)
Full disclosure, at least on my laptop the performance penalty of running this on cargo test seems to be in the noise. (E.g. cargo test vs. cargo test -F wasmparser/try-op.) Probably because the spec testsuite doesn't have many large functions so the cost of cloning is negligible compared with everything else, especially given that this only runs on a debug build and with the feature enabled. But if you think it's better avoided, happy to make a custom feature or cfg for it.
| /// Validates the next operator in a function, rolling back the validator | ||
| /// to its previous state if this is unsuccesful. | ||
| #[cfg(feature = "atomic")] | ||
| pub fn atomic_op(&mut self, offset: usize, operator: &Operator<'_>) -> Result<()> { | ||
| self.validator.begin_atomic_op(); | ||
| let res = self.op(offset, operator); | ||
| if res.is_ok() { | ||
| self.validator.commit(); | ||
| } else { | ||
| self.validator.rollback(); | ||
| } | ||
| res | ||
| } |
There was a problem hiding this comment.
Bikeshedding this a bit, along with the atomic feature from above -- would it perhaps make sense to expose the transaction logic here rather than the singular "just do the one op" method? I'm a bit worried for the same reasons about "atomic_op" sounding like a wasm proposal thing, and this otherwise being more-or-less a duplicate of the op method. By exposing the transaction internals that'd make things a bit more clear here in terms of API contracts and what things are doing (and more composable with more than one op if desired).
Additionally, could you update the documentation of op to state that on failure the internal state of the validator is intentionally not defined in the sense that it can't be reused?
There was a problem hiding this comment.
Unfortunately I don't think we can expose the begin/rollback methods externally because the rollback log is only rich enough to support rolling back one operator. To roll back arbitrarily, I think the log would have to keep track of the order of the various events (e.g. adding a control frame, pushing some operands, adding another control frame, marking the control frame unreachable, etc.) which it's not doing now.
On the name, I changed it to try_op to match the feature name, and updated the documentation for op.
| /// When "atomic" validation of an operator is pending, this is a trace | ||
| /// of discarded info that can restore the OperatorValidator to its | ||
| /// pre-operator state if necessary. | ||
| #[cfg(feature = "atomic")] |
There was a problem hiding this comment.
For Wasmtime I wrote up some guidelines on conditional compilation, notably a small set of stylistic guidelines. One thing I'd recommend here is a change of style here to avoid the sprinkling of #[cfg(feature = "atomic")] throughout if possible.
For example there could be a transaction.rs and a transaction_disabled.rs where both have the same public API but everything in transaction_disabled.rs is a noop. That way this file wouldn't need #[cfg] sprinkled on both use and definition sites, there'd in theory be just one at the mod level.
| #[cfg(feature = "atomic")] | ||
| self.transaction.map(|log| log.record_push()); |
There was a problem hiding this comment.
Personally I find this a little confusing where there's a self.operands.push, a self.record_push method, and a third record_push on the transaction. Here they're all tightly paired together but below the self.record_pop() is much further below the self.transaction.map(...). Would it be possible to refactor this to avoid the duplication here? For example could the self.record_{push,pop} helper methods handle the transaction bits as well? Otherwise it feels a bit weird having up to three different systems to keep in sync with pops/pushes and it's not well centralized at all. The mutation of each one is independent and spread throughout this multi-thousand-line-file as opposed to being in one self-contained location.
There was a problem hiding this comment.
Done (it was a little tricky because of the case where a pop synthesizes a "Bottom" off an empty-but-polymorphic operand stack).
It's fair, but I guess the issue is that we're not building the tool for us -- it's an IDE for other people (especially beginners) to write code in, so I'm sort of thinking about the worst case text that some freshman could paste in that we'd want the tool to handle. :-/ I don't think we'll be encouraging people to write multi-thousand-opcode functions, but I also would like it to be hard for a user to paint themselves in a corner / drive over a performance cliff, hence how I got here...
Got it, and thank you. I appreciate the review and let me take a look at everything. |
2 participants
This PR adds an
atomicfeature to the validator, exposing aFuncValidator::atomic_opfunction that tries to validate an operator and leaves the validator unchanged if the operator is invalid.The feature adds a "rollback log" to the OperatorValidator; the log keeps track of any information destroyed during validation of the operator, so that the validator can be rolled back to its initial state if the operator turns out to be invalid. I tried to make this basically performance-neutral for anybody who doesn't need this. The rollback log is only present if the
atomicfeature is enabled, and it's only used ifatomic_opis being used (i.e. the visit functions andFuncValidator::opdon't use it).We're using this as part of a Wasm development/teaching environment where we'd like the validator to be in a predictable state after an invalid operator.