Skip to content

refactor: small optimization for IsHex method#16310

Closed
shargon wants to merge 1 commit intobitcoin:masterfrom
shargon:master
Closed

refactor: small optimization for IsHex method#16310
shargon wants to merge 1 commit intobitcoin:masterfrom
shargon:master

Conversation

@shargon
Copy link
Copy Markdown

@shargon shargon commented Jun 29, 2019

In case the condition is false, every character is checked first. This operation is not necessary.

If you call the method with a truncated hex string, you will loose performance. The last check will be done, so is better to do it the first one

Comment thread src/util/strencodings.cpp Outdated
@fanquake fanquake changed the title Small optimization for IsHex method refactor: small optimization for IsHex method Jun 30, 2019
@fanquake
Copy link
Copy Markdown
Member

Please fix your commit messages and remove the merge commit.

As this is an optimization, please also provide benchmarks.

Copy link
Copy Markdown
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change supposedly optimizes the false case, I don't think it matters.

@jcliff
Copy link
Copy Markdown

jcliff commented Jun 30, 2019

+1 to @fanquake's recommendation for squashing the commits and fixing the commit message to be clearer.

@fanquake, @promag, This change clearly avoids computation in some cases without adding complexity.

In the case of false, we check every character first, not needed
Small optimization for IsHex method [size==0]
@promag
Copy link
Copy Markdown
Contributor

promag commented Jul 2, 2019

This change clearly avoids computation in some cases without adding complexity.

@jcliff and in other cases adds more "computation". I suspect a benchmark would give similar results, and in that case why bother?

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Jul 3, 2019

Is there any user-visible (or, network-visible) behavior that this would speed up?

Thank you for your first contribution, but we're kind of strict in this project as to requiring a strong rationale for every change. It takes effort to review a change, and there's always a risk.

Comment thread src/util/strencodings.cpp
@laanwj
Copy link
Copy Markdown
Member

laanwj commented Jul 12, 2019

Closing this, sorry, there is no agreement to do this

@laanwj laanwj closed this Jul 12, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants