On Thu, 12 Oct 2023 08:47:09 GMT, Hannes Greule <hgre...@openjdk.org> wrote:
>> More explicitly I was thinking something like below. >> >> I do wonder if some of the benchmark tests should cover the exceptional >> cases. I have seen many systems where attempting to try and parse >> potentially invalid inputs can hit expensive NumberFormatException fallbacks >> (though `-XX:+OmitStackTraceInFastThrow` may eventually kick in). [Guava >> offers `Integer >> Ints.tryParse(String)`](https://guava.dev/releases/32.1.3-jre/api/docs/com/google/common/primitives/Ints.html#tryParse(java.lang.String)) >> and similar to avoid the exception overhead at the cost of boxing for happy >> path of valid `int` and `null` for invalid. >> >> >> checkCanParseLength(s); >> checkCanParseRadix(radix); >> int len = s.length(); >> >> >> private static void checkCanParseLength(String s) { >> if (s == null) { >> throw NumberFormatException.forNull(); >> } >> >> if (s.isEmpty()) { // would be interesting to see if this is >> equivalent to `s.length() == 0` for happy paths >> throw NumberFormatException.forEmpty(); >> } >> } >> >> private static void checkCanParseRadix(int radix) { >> if (radix < Character.MIN_RADIX) { >> throw NumberFormatException.forMinRadix(radix); >> } >> >> if (radix > Character.MAX_RADIX) { >> throw NumberFormatException.forMaxRadix(radix); >> } >> } > > While only optimizing the fast path is a good idea, I think it is important > to make sure there is no regression on the slow path - as @schlosna pointed > out, it's a common way to check if a string can be converted to an int. This patch should have little impact on exception-path performance. Remember String concatenation is done with StringBuilder in java.base, so changing formatter to that might have some performance difference. However, the main overhead of exception was from creation and initialization of stack trace.; the new code was just copies of the existing generic-radix parseInt, except it has a more efficient way to obtain digits via a table. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16112#discussion_r1356510156
