Repository navigation
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
StanFromIreland
changed the title
str creation from non-ASCII data with a branch-free find_max_char()
StanFromIreland
added
the
🔨 test-with-buildbots
This comment was marked as resolved.
This comment was marked as resolved.
bedevere-bot
removed
the
🔨 test-with-buildbots
|
I did some benchmarking with the following script: Detailsimport pyperf
LATIN1 = '\xe9'
UCS2 = '\u019f'
UCS4 = '\U0001f600'
def slice_tail(s):
return s[:-1]
def slice_head(s):
return s[1:]
def slice_head3(s):
return s[3:]
def isascii(b):
return b.isascii()
def decode_latin1(b):
return b.decode('latin-1')
def split_lines(s):
return s.split('\n')
CASES = {
'control: ascii[1001] slice[:-1]': (slice_tail, 'a' * 1001),
'ucs1[8] slice[:-1]': (slice_tail, 'a' * 7 + LATIN1),
'ucs1[21] slice[:-1]': (slice_tail, 'a' * 20 + LATIN1),
'ucs1[1001] slice[:-1]': (slice_tail, 'a' * 1000 + LATIN1),
'ucs1[1001] slice[1:] (odd ptr)': (slice_head, LATIN1 + 'a' * 1000),
'ucs1[1001] slice[3:]': (slice_head3, LATIN1 * 3 + 'a' * 998),
'ucs1[1001] non-ascii body slice[:-1]': (slice_tail, LATIN1 * 1000 + 'a'),
'ucs1[100001] slice[:-1]': (slice_tail, 'a' * 100000 + LATIN1),
'ucs1[10_000_001] slice[:-1]': (slice_tail, 'a' * 10_000_000 + LATIN1),
'ucs2[21] slice[:-1]': (slice_tail, 'a' * 20 + UCS2),
'ucs2[1002] slice[:-1] (issue)': (slice_tail, 'a' * 1000 + UCS2 * 2),
'ucs2[1001] latin1 body slice[:-1]': (slice_tail, LATIN1 * 1000 + UCS2),
'ucs2[1001] slice[1:]': (slice_head, UCS2 + 'a' * 1000),
'ucs2[1001] wide body slice[:-1]': (slice_tail, UCS2 * 1000 + 'a'),
'ucs4[21] slice[:-1]': (slice_tail, 'a' * 20 + UCS4),
'ucs4[1001] slice[:-1]': (slice_tail, 'a' * 1000 + UCS4),
'ucs4[1001] wide body slice[:-1]': (slice_tail, UCS4 * 1000 + 'a'),
'bytes[10] isascii': (isascii, b'a' * 10),
'bytes[1000] isascii': (isascii, b'a' * 1000),
'bytes[1000] decode latin-1': (decode_latin1, b'a' * 1000),
'bytes[1000] non-ascii decode latin-1': (decode_latin1, b'\xe9' * 1000),
'ucs1 200 lines x 51 split': (split_lines, ('abcdefghij' * 5 + LATIN1 + '\n') * 200),
'ucs1 200 lines x 11 split': (split_lines, ('abcdefghij' + LATIN1 + '\n') * 200),
'ucs2 200 lines x 51 split': (split_lines, ('abcdefghij' * 5 + UCS2 + '\n') * 200),
}
runner = pyperf.Runner()
for name, (func, arg) in CASES.items():
runner.bench_func(name, func, arg)AMD Ryzen 7 8845HS,
|
|
So, after some testing I pushed a little change to use four accumulators and a 16-word block so the loop is faster even when it's not vectorised. Benchmarking again, some more promising results: AMD Ryzen 7 8845HS,
|
StanFromIreland
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @StanFromIreland for commit 1daa7ee 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F122902%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
I think I'm pretty happy with this, @vstinner, @eendebakpt and @picnixz what do you think? |
|
The general idea is good, but there are quite some variations possible. The latest commit (small workaround for the compiler not vectorizing) is not great. I tried prompting Claude to come up with simpler approaches. One result is here: pr122902..find-max-char-reduce. Less code, easier to read (imo) and a bit faster (on my noisy system). Benchmark details
Edited by Stan to fix table not rendering. |
|
Hmm, what build were the benchmarks run on? I'll test it. What I'm curious is what happens if the vectorizer does not run? |
|
Benchmarking your branch @eendebakpt, as I feared, on an LTO+PGO build:
And indeed, it was not vectorised, looking at the disassembly: 0000000000621710 <stringlib_find_max_char.lto_priv.0>:
621710: xor %eax,%eax
621712: mov %rsi,%rdx
621715: sub %rdi,%rdx
621718: cmp $0x3f,%rdx
62171c: jle 621727 <stringlib_find_max_char.lto_priv.0+0x17>
62171e: jmp 62173c <stringlib_find_max_char.lto_priv.0+0x2c>
621720: add $0x1,%rdi
621724: or -0x1(%rdi),%al
621727: cmp %rsi,%rdi
62172a: jb 621720 <stringlib_find_max_char.lto_priv.0+0x10>
62172c: test %al,%al
62172e: mov $0xff,%edx
621733: mov $0x7f,%eax
621738: cmovs %edx,%eax
62173b: ret
62173c: xor %edx,%edx
62173e: xor %ecx,%ecx
621740: or (%rdi,%rdx,1),%cl
621743: add $0x1,%rdx
621747: cmp $0x40,%rdx
62174b: jne 621740 <stringlib_find_max_char.lto_priv.0+0x30>
62174d: or %cl,%al
62174f: js 621757 <stringlib_find_max_char.lto_priv.0+0x47>
621751: add $0x40,%rdi
621755: jmp 621712 <stringlib_find_max_char.lto_priv.0+0x2>
621757: mov $0xff,%eax
62175c: ret
62175d: nopl (%rax) |
Windows, MSVC. One thing here is that in the PGO run the path for size > 64 (my branch) or > |
I can try, but I'd really rather not rely on such finicky things. We don't want to slow down platforms that don't vectorize for whatever reason, or strictly rely on the tests (distributors may use different sets for example). |
|
I added a little test, and it didn't work, it likely just wasn't enough. We could try more test cases to emphasise the path, but at that point we're still playing a PGO lottery, and that brings me back to my previous points on this. As such, what do you think about the current approach, which gives a reasonable improvement (and generally no slowdowns, compared to the other patches) even when unvectorized? |
Did you use |
It is a pity we have to do this manual vectorization, but I agree with your arguments so I am leaning towards approving. I found there is already precedent for this approach in On my system using 16-byte accumulators like |
I'm not sure about this I'm afraid, I haven't benchmarked myself but 3% seems like it may be noise. Additionally, it does introduce a fair bit more complexity. I like the current approach as its simpler and more straightforward, if there are concrete ideas in the future we can work on them in follow up PRs? Edit: I pushed a few adjustments based on your branch. |
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
This solution looks quite clear to me (though it's hard to review because of all those interleaved changes on the UI).
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Arithmetic is sound, but I need to convince myself that the last step is enough but not today.
|
Ah I think one of the similar PR was rejected because we didn't really improved much in terms of actual data. However, I find the proposed code simpler so for me it's still a win (we move all preprocessor conditions outside the actual implementations which is better IMO). What'd be interseting is to see whether decoding of actual data on a nosy system makes any difference. The real gain is the vectorization that is now done by the compiler but at the same time, one could say that this could be a separate project (as #120212 eventually concluded) and leave it as is. I still see it as an improvement (the 1.3x geometric mean improvement, even for regular text is worth it IMO and we also added manual vectorizable loops in other modules and this makes the code slightly cleaner). I'm cc'ing @pitrou because he participated in the other discussion just in case. |
|
This PR also significantly speeds up There are a few small cases where the approaches here are a bit slower. E.g. with Overall approved from me as well. |
Change to
find_max_charto speedup string creation.See issue #122901.
find_max_char#122901