refactor: small optimization for IsHex method #16310

pull shargon wants to merge 1 commits into bitcoin:master from shargon:master changing 1 files +2 −1

shargon commented at 10:00 AM on June 29, 2019: none

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
fanquake added the label Refactoring on Jun 29, 2019

in src/util/strencodings.cpp:62 in 3e9654bc31 outdated

  58 | @@ -59,12 +59,13 @@ signed char HexDigit(char c)
  59 |  
  60 |  bool IsHex(const std::string& str)
  61 |  {
  62 | +    if (str.size() <= 0 || str.size()%2 != 0) return false;

shargon commented at 10:10 AM on June 29, 2019:

Choose your favorite:

    if (str.size() == 0 || str.size()%2 != 0) return false;

fanquake renamed this:
~~Small optimization for IsHex method~~
refactor: small optimization for IsHex method
on Jun 30, 2019
fanquake commented at 1:03 AM on June 30, 2019: member

Please fix your commit messages and remove the merge commit.

As this is an optimization, please also provide benchmarks.
promag commented at 8:38 AM on June 30, 2019: member

This change supposedly optimizes the false case, I don't think it matters.
shargon force-pushed on Jun 30, 2019
jcliff commented at 6:24 PM on June 30, 2019: none

+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.

Small optimization for IsHex method

In the case of false, we check every character first, not needed
Small optimization for IsHex method [size==0]

d967411422

shargon force-pushed on Jul 1, 2019
promag commented at 12:46 PM on July 2, 2019: member

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 commented at 9:09 AM on July 3, 2019: member

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.

in src/util/strencodings.cpp:62 in d967411422

  58 | @@ -59,12 +59,13 @@ signed char HexDigit(char c)
  59 |  
  60 |  bool IsHex(const std::string& str)
  61 |  {
  62 | +    if (str.size() == 0 || str.size()%2 != 0) return false;

fqlx commented at 4:01 AM on July 4, 2019:

Please add spaces around the modulus operator.

shargon commented at 10:09 PM on July 5, 2019:

Before was like this

laanwj commented at 5:25 PM on July 12, 2019: member

Closing this, sorry, there is no agreement to do this
laanwj closed this on Jul 12, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels