Repository navigation
Conversation
Using a very simple non-nested scope string, how long does it take to parse?
Using `data[1:]` (where `data` is a string) produces a new string, which necessitates a copy operation. For scope parsing, this is an avoidable cost, since the new string is only used for iteration under `enumerate()` anyway. Replace it with direct use of the enumerator, with an appropriate arithmetic tweak, and we can skip the copy.
Merging this PR will improve performance by 22.68%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_deeply_nested_object_encoding[1w-1d] |
143.4 µs | 116.9 µs | +22.68% |
| 🆕 | test_ordinary_scope_parsing |
N/A | 376.6 µs | N/A |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing sirosen:make-scope-parsing-even-faster (47cc42b) with main (993fe55)
| iterator = enumerate(data, start=-1) | ||
| prev: str | ||
| _, prev = next(iterator) | ||
| for idx, c in iterator: |
There was a problem hiding this comment.
For this kind of micro-optimization, I recommend an explanation like:
| iterator = enumerate(data, start=-1) | |
| prev: str | |
| _, prev = next(iterator) | |
| for idx, c in iterator: | |
| # Equivalent to `for idx, c in enumerate(data[1:])`, but slightly faster. | |
| iterator = enumerate(data, start=-1) | |
| prev: str | |
| _, prev = next(iterator) | |
| for idx, c in iterator: |
Also, is it necessary to state that prev is a string? Its type seems clear, just as c is clear in the for loop.
|
I agree pretty well with both bits of feedback. A comment does seem worthwhile, and maybe we don't need to write down the type of the string we get from iteration. However, I'm self-closing because I think the CI benchmark run shows that I was not accurately measuring things when I tried this locally. I saw a speedup that looked like it was in the 5-10% range on some benchmarks, and it just doesn't reproduce. Kurt and I had a brief conversation about this and I plan to pull out separate, dedicated benchmark changes. Not only do I like my new benchmark, but I also think the one which shows change here is too fragile. I still feel instinctively like this bit of parsing code could be made faster, but short of busting out |
2 participants
I noticed that we took a string slice (
data[1:]) in a place where it was avoidable.A new benchmark showed me in local runs that avoiding the slice could save a little bit of time in realistic cases, although it's not much. I'm curious if codspeed benchmarks will show a clearer win for this change.