Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Optimize IsSpace function for common non-whitespace characters #29602

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paplorinc
Copy link
Contributor

@paplorinc paplorinc commented Mar 8, 2024

The IsSpace function has been optimized for the more common case where a character is not whitespace. Previously, the function checked each whitespace character individually, which was not efficient for non-whitespace inputs as it required multiple comparisons.
This method is often used for parsing various inputs more flexibly, see usages.
This results in an additional ~2% performance improvement for base conversions (see related hexadecimal and base58 optimizations).

The updated IsSpace function now first checks if the character is less than or equal to ' ' (the space character, which has the highest ASCII value among the whitespace characters). This single condition can quickly determine that most characters (i.e. letters, numbers) are not whitespace, thus short-circuiting the evaluation for the most common case.

Otherwise the function performs additional checks to see if it is either a space character or within the range of horizontal tab to carriage return ('\t' to '\r'), which are the remaining whitespace characters in the ASCII table.

This change assumes that most calls to IsSpace are for non-whitespace characters, as can be inferred from the usage patterns where IsSpace is often used in loops that parse strings until a non-whitespace character is encountered.

To make sure the changes keep the previous functionality, I've added an exhaustive unit test for it.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 8, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@maflcko
Copy link
Member

maflcko commented Mar 15, 2024

Is this used in a hot path that is relevant, to warrant the changes?

@paplorinc
Copy link
Contributor Author

Is this used in a hot path that is relevant, to warrant the changes?

It's used in base 58 and 16 conversions while iterating over each character of the input.
We're expecting most of them to not be spaces, it would make sense to optimize for that scenario and exit early.

@maflcko
Copy link
Member

maflcko commented Mar 15, 2024

Yes, but base58 isn't used in a hot path, where optimizing it would result in a noticeable difference. (When sending an address over RPC, the remaining overhead is too large to notice a speedup of a few nanoseconds)

@paplorinc
Copy link
Contributor Author

paplorinc commented Mar 15, 2024

It's a small improvement, indeed, but it eliminates useless work, done frequently.
The exhaustive tests make sure the behavior is exactly the same as before, so while it may be considered low-reward, it should be low-risk as well.
Is there any area (testing, documentation, code) that you would like me to improve to increase the risk/reward ratio?

@laanwj
Copy link
Member

laanwj commented Apr 9, 2024

Is this really a bottleneck? A benchmark would be useful to see if this isn't something the compiler already does.

ACK on the test.

@paplorinc
Copy link
Contributor Author

Thanks for the review @laanwj. The gain is small, but measurable:

make && ./src/bench/bench_bitcoin --filter=HexParse --min-time=10000

Before:

|           ns/base16 |            base16/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|                0.68 |    1,461,990,359.30 |    0.0% |     10.83 | `HexParse`
|                0.68 |    1,461,442,745.57 |    0.1% |     10.83 | `HexParse`
|                0.68 |    1,461,262,276.18 |    0.1% |     10.83 | `HexParse`

After:

|           ns/base16 |            base16/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|                0.67 |    1,495,985,893.94 |    0.0% |     10.82 | `HexParse`
|                0.67 |    1,494,916,055.36 |    0.1% |     10.82 | `HexParse`
|                0.67 |    1,494,545,662.66 |    0.0% |     10.82 | `HexParse`

which results in a speedup of ~2% for 130 non-whitespace hexadecimal characters:

(1,495,985,893.94 / 1,461,990,359.30) = 2.3%

@paplorinc paplorinc force-pushed the paplorinc/is_space_optimization branch from 210a280 to 3a47a61 Compare April 9, 2024 10:38
The IsSpace function has been optimized for the more common case where a character is not whitespace. Previously, the function checked each whitespace character individually, which was not efficient for non-whitespace inputs as it required multiple comparisons.

The updated IsSpace function now first checks if the character is less than or equal to ' ' (the space character, which has the highest ASCII value among the whitespace characters). This single condition can quickly determine that most characters (those with ASCII values greater than ' ') are not whitespace, thus short-circuiting the evaluation for the most common case.

If the character is less than or equal to ' ', the function performs additional checks to see if it is either a space character or within the range of horizontal tab to carriage return ('\t' to '\r'), which are the remaining whitespace characters in the ASCII table.

This change assumes that most calls to IsSpace are for non-whitespace characters, as can be inferred from the usage patterns where IsSpace is often used in loops that parse strings until a non-whitespace character is encountered.
@paplorinc paplorinc force-pushed the paplorinc/is_space_optimization branch from 3a47a61 to e4e4c36 Compare May 11, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants