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
Member
|
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,
|
Member
|
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
Member
|
I think I'm pretty happy with this, @vstinner, @eendebakpt and @picnixz what do you think? |
Contributor
|
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. |
Member
|
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? |
Member
|
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) |
Contributor
Windows, MSVC. One thing here is that in the PGO run the path for size > 64 (my branch) or > |
Member
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). |
Member
|
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? |
Member
Did you use |
This file contains hidden or 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
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.
Change to
find_max_charto speedup string creation.See issue #122901.
find_max_char#122901