Add #[inline] and #repr[transparent] to ComponentIdSet to attempt to fix perf regression#23471
Conversation
|
@mockersf do you want to try merging this and see what happens? |
|
I might suggest adding |
#[inline] attributes to trait impls for ComponentIdSet#[inline] and #repr[transparent] to ComponentIdSet to attempt to fix perf regression
alice-i-cecile
left a comment
There was a problem hiding this comment.
I have no complaints about doing this, and I'm fine to let our automated benchmarking test this.
|
|
||
| #[inline] | ||
| fn next(&mut self) -> Option<Self::Item> { | ||
| self.0.next().map(ComponentId::new) |
There was a problem hiding this comment.
I think .map doesn't always output the best gen code (I need to check with godbolt), and I keep seeing other codebases that a match statement is better for codegen/optimisation. It's very small improvements, but I saw some of it with me optimising my nailpit project.
Example from my codebase. This form ended up being the "best" from what I could profile/benchmark for this part of the codebase.
These matter more for code in hot-loops, which would be the iterator .next code, etc
There was a problem hiding this comment.
There was a problem hiding this comment.
I added -C opt-level=3 -C target-cpu=x86-64-v3 to each, and they both compile down to the same optimised asm:
map: https://godbolt.org/z/nMzrbT834
match: https://godbolt.org/z/dhbKv4MEh
I think the .map advice is more for optimising compile times rather than runtime speed, by the looks of it.
Objective
Attempt to fix a performance regression from adding the
ComponentSettype in #23384. See #23464.Solution
Add
#[inline]attributes to the new trait impls. I had added them to the inherent methods, but forgot about the traits!Testing
I have not tested this at all, so I don't know whether it will actually fix the issue! I don't even understand how #23384 could have affected those benchmarks in the first place, since they don't seem to call any methods on
Accessduring the loop.But these trait methods all simply delegate to methods on
FixedBitSet, so marking them#[inline]seems harmless at worst.