-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rollback iterator to avoid cloning tokens #41
Comments
Some scattered thoughts:
|
There's currently no infrastructure for benchmarks in the library, but I remember you did some benchmarking on your own in the past. I assume those were mostly ad-hoc scripts/commands and not something you could add in a
venial's tree-like architecture means that even specialized parsing methods (that might be rollbacked) typically just act as a part of a large What you suggest would mean that we could convert So I see only two options: clone the iterator (current approach), or transparently retain a buffer in What we can do is have another iterator type during the "attempt" phase where a backup point is set. This is more for type safety (type state pattern) and micro-optimization (less branches), not a semantic requirement. My hope is that such iterators would be declared locally and not infest many function signatures, otherwise we might need to think about a runtime flag or make many functions generic.
Yes, they are both very similar and can be implemented with an internal
Yes, a very low-hanging fruit would just be to use |
Yes, that's what I was thinking. I was mostly thinking about the "micro-optimization" part (I'd rather not add a branch to every single token read if they're only needed for a very small subset of the parsed programs).
I guess it depends on whether we can estimate the lookahead needed and we find that it's always a small number. In that case then, yeah, a small static buffer would make sense.
Yup, it was mostly ad-hoc. I guess making a benches directory would be a good idea regardless of this issue. |
@PoignardAzur Random idea: Could the rollback iterator be made by injecting E.g. quote! {
let #rollback_rand_ident = {
if false {
const rollback_ty = gen_placeholder_type!();
<BEGIN_#rollback_rand_ident as #rollback_ty >::new()
} else {
() as _
}
};
} |
I'm not sure I visualize what you're proposing, but that doesn't really look like a solution that fits our problem space. For one thing, there will be places where we want to rollback that aren't expressions and where the grammar wouldn't accept those |
We have now 4 occurrences of this:
// TODO consider multiple-lookahead instead of potentially cloning many tokens
And in the last days I was thinking how to approach that best. First I had the idea of a multi-peek iterator (which would allow looking ahead several tokens instead of just 1). While thinking how to apply this in venial, another idea came up, which seemed more ergonomic to me: a rollback iterator, meaning that you would normally call
next()
but could rollback at any time to a previously set backup point.Rough sketch:
(it's also possible to have explicit commit and rollback-on-drop instead)
This can be implemented via
VecDeque
, which keeps the tokens since the backup point in the buffer. When rolling back, the iterator wouldpop_front()
the buffer first, before callingnext()
on the underlying iterator. Otherwise it would advance as usual.One thing to note: for rollbacks to work, the basic
TokenIter
type which is currently used everywhere would have to be a different iterator type that supports this buffer. It's not possible to only selectively use a rollback iterator in the places where rollback is used, because the more generalTokenIter
cannot rewind. So we would need to think which of theIterator
methods are worth overriding. Probablysize_hint()
, but I'm a bit reluctant to touch all the others for possible optimization, at the big risk of introducing bugs.What do you think about this?
The text was updated successfully, but these errors were encountered: