-
Notifications
You must be signed in to change notification settings - Fork 18k
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
strconv: allocation in the error case dominates the runtime #43241
Comments
Why, though? There's no reason to expect that, really. Sometimes on invalid input you'll need to do something different than what you do in the happy path (e.g. to create a descriptive error value to return). I don't think scrapping the current custom error values just to shave off 40ns in the error path is a good idea. Also this change is not backward compatible. You can't break existing Go code, there's a Go 1 Promise of Compatibility. |
I'm not suggesting breaking anything, I haven't suggested anything, really, I just stated the issue. My implementation is not a proposal for code change, it's there to illustrate what the maximum perf gain potential is. |
I have to agree with @ALTree; building a proper error has a small cost but it's the right choice. Have you tried tweaking the code to reduce the cost of building the error? I imagine you won't gain much though, as it's a matter of nanoseconds. In general, I think that if you want a very low level API that trades correctness for performance, that can always live in a third party module. You might be interested in #42429, which also touches on allocs for this API. |
The way I see it there are at least four options here:
@ALTree suggested I'm all for the last option, but I actually tried fixing this properly (and failed), hence this issue where I'm trying to raise it to see if options 2/3 will be feasible, thanks to input by someone who's more versed in allocation logic in Go. If options 2/3 are not feasible (entirely possible), we can either close this (option 1) or we go with option 4 and tag this Go2 and leave it for further discussion when the question of stdlib evolution is debated again. (Yes, @mvdan, I'm very much interested in the issue you link to and have been subscribed for a while. I'm looking forward to it, it's a much higher source of allocations for me than this one.) |
Allocating the NumError is not avoidable.
The allocation is necessary. We are giving back the user a new NumError which must be allocated somewhere.
I don't think this will be the right tradeoff for strconv. It's much nicer when the error you get back has the context inside it so every caller doesn't need to separately annotate the error information. In most scenarios where the performance of strconv.ParseInt matters a lot, inputs tend to be valid, and the performance of the error case isn't a large concern. If you have a use case where you need to parse a huge volume of not-numbers efficiently, you may well need to turn to custom code that makes a different tradeoff in terms of good errors vs. allocations. (I disagree with @mvdan that this is about trading correctness for performance, however.) |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes, even on yesterday's tip.
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
I ran
strconv.ParseInt
(andParseFloat
) on valid input, e.g.123
, and invalid input, e.g.foo
.What did you expect to see?
I expected comparable performance for both valid and invalid inputs.
What did you see instead?
In the error case, ParseXXX functions allocate and that dominates the runtime. I tried replacing all the custom errors with a simple
var errNotInt = errors.New("not an int")
and got this:The benchmark code:
The library code (taking
strconv
and changing its error handling and putting it all in one file):The text was updated successfully, but these errors were encountered: