Skip to content

Conversation

@evantypanski
Copy link
Contributor

@evantypanski evantypanski commented Dec 12, 2025

Since the two benchmarks used the same Inner, they would get the same interprocedural optimizations. This doesn't give the full picture about how something changes, so separate them so that we can get a better picture.

This definitely makes the benchmarks more "micro," but I believe that we see more people using either lookahead or size, not both on the same unit.

I added a second commit that completely eliminates the need for lookahead in the size benchmark, but I'm not entirely sure that it's fair. I actually think it might be. The problem is that even if lookahead wasn't obviously used, having the literals triggers lookahead. I want it to not use lookahead so that I can have different optimizations for each, but I also understand it's kind of forcing benchmarks to say what I want them to say. My preference would be the lookahead and size benchmarks actually test different things, though, so I don't really mind it - just take the initial results with a grain of salt if we keep it!!

EDIT: And I just looked through Zeek's parsers before/after #2228, there are plenty of hot paths that get their lookaheads removed. I'd like that expressed in some of our benchmarks.

Since the two benchmarks used the same `Inner`, they would get the same
interprocedural optimizations. This doesn't give the full picture about
how something changes, so separate them so that we can get a better
picture.

This definitely makes the benchmarks more "micro," but I believe that we
see more people using *either* lookahead *or* size, not both on the same
unit.
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 12, 2025

CodSpeed Performance Report

Merging #2229 will degrade performances by 7.22%

Comparing topic/etyp/change-inner-benchmarks (4b49454) with main (bdd9092)

Summary

❌ 4 regressions
✅ 24 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
UnitVectorSize][10000] 12.3 ms 13.2 ms -7.22%
UnitVectorSize][1000] 1.2 ms 1.3 ms -7.22%
UnitVectorSize][100000] 129.8 ms 137.1 ms -5.31%
UnitVectorSize][100] 157.4 µs 169.1 µs -6.87%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants