Conversation
|
I'm trying so hard to remember why I didn't do this to begin with, so I will not merge immediately until I either remember why, or decide that it doesn't matter. In any case, I consider removing |
|
Oh and you forgot to update the testcases with the rank1 constant, though this is irrelevant if you revert that change. |
This reverts commit b2cd1bd.
|
Yeah I wasn't sure about that one either. reverted. |
|
I measure a performance degradation of 5-10% for rank1. |
|
It is unclear to me where the performance hit is coming from. It is possible that this is just an artifact of the microbenchmark, though, iirc, I originally didn't use this approach because I was already measuring performance degradation back then, which was on a different setup and so benchmarking artifacts are less likely. It should be noted that this PR only affects code readability, you are not actually improving branching, as the parameter the branches depend on is constant. Still, I'd expect this to be as fast as the current implementation, but at least in the microbenchmark it is consistently slower. |
|
Well, even after retrying, i cant get this to work without a performance regression. I don't know if the regression is real, because I don't have realistic benchmarks at hand. I will close this until it can be shown that the regression is either a weirdly reproducable artifact of microbenchmarking, or a version of this can be found that doesn't regress. |
2 participants
The idea is that to calculate rank1 we can just subtract rank0 from the current position.
This lets us simplify the rank() function's logic d163337 and not have to keep track of redundant data b2cd1bd
I made sure to run the tests and benchmarks, to ensure that there were no performance regressions.