More Strings replaced with &strs
#579
Conversation
Unfortunately due to borrow checker limitations, this required moving `input` fields out of both `Parser` and `Tokenizer`, as with the immutable borrow in place, there is no way to tell Rust that a mutable borrow won't touch the input So, the input is now an (immutable ref) argument to all the methods that used to refer to `&self.input` And there were a lot... But now `Token` doesn't carry an owned `String` Also shrunk `Tokenizer` by doing all math in terms of byte offsets into its input (using existing `SourceCodePosition` fields) instead of storing a separate `Vec<char>` with char indices
… used on a newline and would've had no effect, it is semantically correct to keep the original `+= 1`)
…self.context.lock()` as the lock must be dropped to allow `self` to be borrowed in the interm)
…ut where possible Note that this generally means the input must actually be longer lived than the evaluated stuff (not a problem except in testing, evidently)
There was a problem hiding this comment.
Thank you very much for this! I'm currently away from home but I plan to do some benchmarks soon to see if your changes result in performance improvements. They definitely result in memory improvements and save a ton of allocations which certainly also improves performance, especially on platforms with slow allocators (wasm?).
numbat/src/prefix_transformer.rs
Outdated
| let statements = statements.into_iter(); | ||
| let mut ans = Vec::with_capacity(statements.size_hint().1.unwrap_or(0)); | ||
| for s in statements { | ||
| ans.push(self.transform_statement(s)?); | ||
| } |
There was a problem hiding this comment.
Is this a performance improvement or was it somehow necessary to rewrite this iterator expression into a loop for another reason?
There was a problem hiding this comment.
Probably a vestige of me trying to fix a borrow checker error. Reverted.
numbat/src/tokenizer.rs
Outdated
| @@ -144,7 +144,7 @@ pub enum TokenKind { | |||
| Eof, | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | |||
There was a problem hiding this comment.
Why this change? Even with your recent improvements, Token is still > 24 byte large(?). Maybe it's better to keep clones of this explicit, by not deriving Copy?
There was a problem hiding this comment.
Probably a vestige of me trying to fix a borrow checker error. Reverted.
According to hyperfine, |
Yes. This is mostly due to the performance improvements documented in #525.
I don't think so. I did a few experiments, and it seems like your allocation-reducing PRs had no measurable runtime effect on x86. This does not mean that they were useless. As I said before, I could imagine that they help on other platforms (WASM). But I'm not sure how to measure performance in the browser. And your changes definitely help with memory consumption, which is an interesting metric in itself. Especially on non-desktop devices. Or when Numbat is used as a library in other applications. |
|
Ok, I'll take this back. There is a small performance improvement. I just checked out 29a99e9 from Aug 29, before any of your changes: and compared it with The results are here:
There seems to be a statistically-significant 5% performance improvement 👍 I ran both versions with the prelude from 29a99e9 for a fair comparison. |
I think this is the key thing that I did wrong previously. If I measure Some of this increase is from a new |
A decent heuristic is that anything only used during parsing and not stored long term in the Environment can borrow from the input.