Introduce ARM Neon and SSE2 SIMD. #743
Conversation
|
The gain seem to be 7% on real word benchmarks: Also note that I did one more refactoring to make the introduction of SIMD easier, so you still have a conflict. |
ext/json/ext/generator/simd.h
Outdated
| uint8x16x4_t load_uint8x16_4(const unsigned char *table, int offset) { | ||
| uint8x16x4_t tab; | ||
| for(int i=0; i<4; i++) { | ||
| tab.val[i] = vld1q_u8(table+offset+(i*16)); | ||
| } | ||
| return tab; | ||
| } |
There was a problem hiding this comment.
Isn't that just vld4q_u8?
https://developer.arm.com/architectures/instruction-sets/intrinsics/vld4q_u8
There was a problem hiding this comment.
Unfortunately it's not. vld4q_u8 interleaves the data among the 4 vector registers.
% cat load-test.c
#include <stdio.h>
#include <stdint.h>
#include <arm_neon.h>
void print_vec(char *msg, uint8x16_t vec) {
printf("%s\n[ ", msg);
uint8_t store[16] = {0};
vst1q_u8(store, vec);
for(int i=0; i<16; i++) {
printf("%3d ", store[i]);
}
printf("]\n");
}
uint8x16x4_t load_table(uint8_t *table, int offset) {
uint8x16x4_t tab;
for(int i=0; i<4; i++) {
tab.val[i] = vld1q_u8(table+offset+(i*16));
}
return tab;
}
int main(void) {
uint8_t table[256];
for(int i=0; i<256; i++) {
table[i] = i;
}
uint8x16x4_t tab1 = load_table(table, 0);
print_vec("tab1.val[0]", tab1.val[0]);
print_vec("tab1.val[1]", tab1.val[1]);
print_vec("tab1.val[2]", tab1.val[2]);
print_vec("tab1.val[3]", tab1.val[3]);
printf("\n");
uint8x16x4_t tab1_2 = vld4q_u8(table);
print_vec("tab1_2.val[0]", tab1_2.val[0]);
print_vec("tab1_2.val[1]", tab1_2.val[1]);
print_vec("tab1_2.val[2]", tab1_2.val[2]);
print_vec("tab1_2.val[3]", tab1_2.val[3]);
return 0;
}
% ./load-test
tab1.val[0]
[ 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 ]
tab1.val[1]
[ 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 ]
tab1.val[2]
[ 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 ]
tab1.val[3]
[ 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 ]
tab1_2.val[0]
[ 0 4 8 12 16 20 24 28 32 36 40 44 48 52 56 60 ]
tab1_2.val[1]
[ 1 5 9 13 17 21 25 29 33 37 41 45 49 53 57 61 ]
tab1_2.val[2]
[ 2 6 10 14 18 22 26 30 34 38 42 46 50 54 58 62 ]
tab1_2.val[3]
[ 3 7 11 15 19 23 27 31 35 39 43 47 51 55 59 63 ]
There was a problem hiding this comment.
Wow, that's so weird.
Well, maybe that loop should be unrolled then, I suspect the compiler does it, but might as well be explicit.
|
Can you just include the implementation for the regular escaping? I'm not sure the script safe version is quite worth it. |
…tion. Also store the potential matches directly rather than looking up values in the escape table.
ext/json/ext/generator/generator.c
Outdated
| if ((ch_len = search_escape_basic_neon_advance_lut(search)) != 0) { | ||
| return ch_len; | ||
| } | ||
|
|
||
| // if ((ch_len = search_escape_basic_neon_advance_rules(search)) != 0) { | ||
| // return ch_len; | ||
| // } |
There was a problem hiding this comment.
Seems like it's a toss up which one is the best. It might be an artifact that my M1 Macbook Air is passively cooled and it gets warm after I run it over and over.
|
Comparison between Running it a second time: |
…e only need 128 bytes for the lookup table as the top 128 bytes are all zeros.
|
Not sure why but it's way more modest on my machine (Air M3): |
|
Apologies for going dark for a while. I've been trying to make incremental improvements on a different branch (found here). My hope was using a move mask would be faster than Feel free to try it out though. |
That's no worries at all. I want to release a After that I think I can start merging some SIMD stuff. I'd like to go with the smaller possible useful SIMD acceleration to ensure it doesn't cause issues with people. If it works well, we can then go farther. So yeah, no rush. |
|
@byroot if you have a few minutes, would you be able to checkout this branch and benchmark it against master. You'll have to tweak your compare script a bit to compile this branch with This branch uses the bit twiddling sort of platform agnostic SIMD code if the SIMD code is disabled via a The results on my M1: |
|
With that compilation flag and compared to |
|
From a co-worker with an M4 Pro: |
|
From another co-worker with an M1 Pro: |
|
I just pushed a PR #769 to this repo which also employs SIMD to speed up string escapes. I am really sorry that we both worked in that area at the same time; after I started my work I didn't check back with this repo for a while (and I should have done that.) I believe the main difference between my PR and yours seem that mine supports x86 as well. It is doing this by using a cross-platform shim I want to suggest to collaborate on getting SIMD support in one way or another. 👋 |
|
Hi @radiospiel, I'll take a look at #769. I originally started working on #730 which supports Neon, SSE 4.2 and AVX2 with runtime detection support. The PR got a bit big so I decided to close it and implement each instruction set individually. Additionally, @byroot refactored the code quite a bit to make the SIMD implementation quite a bit easier. There are two implementations in this PR, one uses a lookup table and the other is rule-based. Both seem to have similar performance on my machine. On my machine I see a 11%-48% improvement depending on the benchmark. A few of my co-workers saw various speedups depending on their machine. I should probably mark this PR as "Ready for Review". However, I'm happy to collaborate either on this or your PR. Edit: oh yeah, there is an old-school bit-twiddling SIMD approach in pure C: #738 |
|
Thank you, @samyron . I became painfully aware of the work you did when I tried to merge master into my branch, because the interface's of the escape functions had been changed; my implementation relies on a "escape me a The main difference between your approach and mine is that you switch out the search functionality, depending on the availability of SIMD, while I switch out the SIMD primitives instead. This allows me to have working implementations for X86, ARM, and bit-twiddling; but only a handful of primitives are available because NEON and AVX are different, so your approach should allow for per-hardware type optimal implementations. I have a busy week ahead of me, but I will definitively take a look end of the week. I will also benchmark on Graviton instances; most ARM server workloads are probably not on a Apple Silicon CPU after all :) Happy to benchmark this PR as well. Can you share a benchmark script that produces the most useful output for you? I would be especially interested in understanding how you get the "before" and "after" entries in the benchmark output :) Speaking of benchmarks:
This is magnitudes more than the numbers posted here. I have seen a 48% posted above (on the |
Apologies, yes, that was a typo. I'll fix it in the comment above |
|
@samyron I reran benchmarks (link). Both our PRs show a substantial improvement over the baseline, the only significant difference is on short strings.
strings.short is a test on a 13-byte string I believe such short strings are relevant, because JSON object keys are probably quite often shorter than 16 byte; my PR applies SIMD for strings of 8 byte and more (link). (The value of 8 seemed beneficial and looked nice, but I should probably retest this with smaller values.) Maybe you could be able to support that as well? |
|
@byroot we have two competing implementations of the same approach. While mine is probably more beneficial in the short term (because it also supports x86), I believe that @samyron 's approach has more future potential, because it allows handcrafted SIMD implementations that are fundamentally different between NEON and SSE2. (and it certainly can be extended to also support shorter strings, see comment above.) Also, transplanting a x86 implementation from my PR into @samyron 's shouldn't be too hard to achieve. I see the following alternatives:
What do you all think about that? ☝️ |
| #ifdef ENABLE_SIMD | ||
|
|
||
| #if defined(__ARM_NEON) || defined(__ARM_NEON__) || defined(__aarch64__) || defined(_M_ARM64) | ||
| #include <arm_neon.h> |
There was a problem hiding this comment.
The internet is convinced that no ARM64's lack neon support, so maybe this is not necessary. For example here: https://github.com/postgres/postgres/blob/REL_17_4/src/include/port/simd.h#L38
Wow, I had not expected that to make a difference. Great finding! I was running your latest commit on a Intel(R) Xeon(R) Platinum 8175M CPU @ 2.50GHz. I can confirm that worst case performance is no longer adversely affected also in x86. Here are the numbers for the strings benchmarks; this is on a ruby 3.3. (I can't compare against 2.10.2 or a newer ruby, because that is not available there.)
so a 30% speedup on string focused benchmark cases. |
|
Somehow, I get that because we need to check that we're not going out of bounds it slows things down a bit, but 9% seem a bit surprising. |
|
As for code review I wanted to ask about two things:
|
Thank you! Done. |
is this PR rebased off the current master? maybe the test case benefits from other optimizations that have landed in master but are not present here? |
ext/json/ext/generator/generator.c
Outdated
| memset(s, 'X', len); | ||
|
|
||
| // Optimistically copy the remaining 'len' characters to the output FBuffer. If there are no characters | ||
| // to escape, then everything ends up in the correct spot. Otherwise it was convenient temporary storage. | ||
| memcpy(s, search->ptr, len); |
There was a problem hiding this comment.
I don't understand what this memset purpose is, given we memcpy at the same pointer with same length right after.
If I comment it out, the test suite still pass on my machine. Is it some sort of left over?
There was a problem hiding this comment.
That's a bug/typo. That was supposed to be memset(s, 'X', vec_len);. The goal is to ensure that the anything past len bytes does not need to be escaped.
No. But I tried to merge master into it in case you were right, and I still have a 10% slowdown on |
|
I see a 3% performance improvement (but within the error ranges) with this branch. These are the numbers I see on citm_catalog (Apple M1, 3.4 + yjit): When I change the 8 byte limit to 6, (i.e. we SIMD even strings of only 6 bytes) I get and with 4 I get Finding the optimum for this limit might be heavily CPU model specific, though, so I don't know how far we want to go there. |
vector's width worth of data remaining.
|
In the case there isn't a vector's width worth of data, but at least Edit: Updated the code for SSE2 too. The real-world benchmarks on my M1 are now: |
…ector's width worth of data.
|
I now have access to an M4 Pro. Here are the current real-world results compared to |
|
interesting to see that on the m4 pro the difference between SIMD and non-SIMD is less prominent (but still very much there.) |
|
This branch is still work in progress but it optimizes the worst cases. I don't like that I had to create an The key idea is to use Comparing that branch with Note: At this point my M1 might be thermal throttling. I've been running benchmarks almost constantly for a few hours. |
Interesting. Have you tried Also just a quick heads up, I'm gonna be busy with RubyKaigi for the next ~10 days, so don't be surprised if I'm not responsive. Once you are satisfied with the performance and wish to prepare this PR to be mergeable, you can get ris of the disabled codepath, commented out code, etc. I have a few diff comments that weren't addressed. |
I haven't but I will try and see what happens.
I think now that the worst case is addressed to not be worse than I'll also ensure code style consistency... I know I've been putting my opening |
|
I have run benchmarks on servers (they shouldn't be affected by thermal throttling as much.) Here are the numbers, comparing 2.3.1 → 2.7.6 → 2.9.1 → current master → this branch (at commit ad995fc), on both Intel X86 and ARM Neon machines. ruby 3.3.4; 16 core Intel(R) Xeon(R) Platinum 8175M CPU @ 2.50GHz
ruby 3.3.4; 16 core Graviton CPU @ 2.10GHz
There is still quite a drop in performance on the citm_catalog testcase, unfortunately. activitypub benefits nicely, and we have some gains on twitter. |
|
Note that these numbers ^^^ are on a ruby 3.3.4; ruby 3.4.1 is not available on those machines. |
This comment was marked as outdated.
This comment was marked as outdated.
but it looks like that effect disappears when the current master is merged into this PR. I'll rerun benchmarks again and post results. |
|
@radiospiel thank you so much for the benchmarks. I'm currently focused on the SSE2 implementation, particularly around the worst case performance using this benchmark: With the code as of commit
Instead of using the function pointer I'd be much happier with a 10-15% performance decrease compared to master rather than an 50%. Trying a simple conditional: I get the following performance: I need to test this on my M1. Ideally I'd like to keep the implementation the same between platforms. |
|
As of master commit Combined Real-world and Synthetic Benchmarks |
Version 2 of the introduction of ARM Neon SIMD.
There are currently two implementations:
Benchmarks (Lookup table)
Benchmarks (Rules based)
I am still working on this but I wanted to share progress.
Edit: Looks like I missed one commit so I'll have to resolve some merge conflicts.