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

[Java] Make AsciiEncoding throw exceptions similar to JDK parseInt/parseLong #214

Conversation

AndreyNudko
Copy link
Contributor

Empty string results in NumberFormatException. Attempt to read outside string boundaries (or negative length) in IndexOutOfBoundsException.
This behaviour is consistent with Integer::parseInt / Long::parseLong, and makes a slightly more usable API which does not force callers to catch 2 exceptions for the same kind of error - non-numeric input value. Incorrect usage still results in IndexOutOfBoundsException.

As I understand this is performance-sensitive call, I've run some benchmarks to parse 1 million numbers and try to evaluate effect of the special-case for length <=1

  • mix of random 1- and 2- digit numbers (80/20)
  • random numbers which are multiple of 1 million in 1-50m range
  • random non-negative numbers
  • mix of random negative and non-negative numbers
    On my machine first case actually shows slight improvement (~15%) - which I don't quite understand. The other 3 produce pretty consistent results on both original and updated code.

Benchmarks are not included in this PR, but are available from https://github.com/AndreyNudko/agrona/tree/ascii-encoding-benchmarks/agrona-benchmarks/src/main/java/org/agrona

@mjpt777 mjpt777 merged commit 33d5da3 into real-logic:master Jul 20, 2020
mjpt777 added a commit that referenced this pull request Jul 20, 2020
@AndreyNudko AndreyNudko deleted the ascii-encoding-throw-number-format-on-empty-string branch October 4, 2020 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants