Moving to use a single lookup table #116
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
As expected, on my Intel performance is not WOW compared to #116, but decent enough to justify merging this - the Ryzen improvement is just too large to ignore it. |
|
On Apple M2 Pro the on |
|
thanks @lucamolteni as expected - since Apple have lot of parallelism on loads, the data-dependent operation using a single table, make it a bit slower :"( |
|
@lemire this new version is slightly slower on M1/M2 then master, but still immensely better than the original table approach ❤️ |
This was referenced Jan 10, 2025
|
What you think @lemire ? This pr shine on x86 AMD but not on Arm, master, the opposite |
|
@franz1981 Thanks for the comments. I will come back to this issue at a later date. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is funny here - on Ryzen, the existing code produce this performance
which is decent and near to what can be obtained with the existing table approach when there are no special chars - but still is underperforming compared to the whoopy 33K with newish Intel - which seems able to perform 2 lookups in parallel in the same cycle, for each read char.
For comparison - the closed pr at #114 with Ryzen, was overperforming the existing approach with Intel - even if not by a great margin i.e. ~28K ops/sec
Moving to a single table lookup with Ryzen (4) the performance is the best we could achieve i.e.
IPC now is much better and
L1-dcache-loadsare way less - as expected.I'll give it a shot on Intel to see how it performs - but I start to think we're moving in a land where the CPU frontend design matter enough to give very different outcome with small changes in code
Just for reference., the assembly produced by this version is not really great - which surprise me that it gives so much better performance...
Still an unrolling of 4, but more xmm* spilling, likely due to register pressure