Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Fri, 13 Aug 2021 13:16:04 GMT, SkateScout
wrote:
> OK even if we keep out the edge case in the try block the
> "parseLong(nm.substring(index), radix)" could be replaced as already
> mentioned with parseLong(nm. index, nm.length(), radix)
I think it already was:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Long.java#L1287
> and in the catch block the idea to throw an "nice" error can be misleading.
> since -02147483648 for example would become -2147483648 because the radix is
> 8.
Yes, it's slightly misleading that the radix specifier gets cropped out, but
the error message does include the radix information so it's not a bug
technically:
jshell> Integer.decode("-01234567890123");
| Exception java.lang.NumberFormatException: For input string:
"-1234567890123" under radix 8
> Since per Radix only one String is possible to get through would if not be
> faster and more clear to check (compare) if it is the matching string and
> return the correct value else throw the error. This is also easy to read and
> even if is on the edge avoid substring , concationation and reparsing.
It might be a bit faster for that one non-exceptional accepted input, sure. It
could also incur a cost on the fast path due increasing the weight of the code.
You'd need to carefully measure that the added logic and checks doesn't cause
any trouble elsewhere.
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Tue, 10 Aug 2021 21:06:00 GMT, Сергей Цыпанов wrote: >> The code in `Integer.decode()` and `Long.decode()` might allocate two >> instances of Integer/Long for the negative values less than -127: >> >> Integer result; >> >> result = Integer.valueOf(nm.substring(index), radix); >> result = negative ? Integer.valueOf(-result.intValue()) : result; >> >> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` >> method. Same applicable for `Long` and some other classes. > > Сергей Цыпанов has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8267844: Add benchmarks for Integer/Long.decode() > - 8267844: Rid useless substring when calling Integer/Long.parse*() > - Merge branch 'master' into 8267844 > - Merge branch 'master' into 8267844 > - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where > applicable OK even if we keep out the edge case in the try block the "parseLong(nm.substring(index), radix)" could be replaced as already mentioned with parseLong(nm. index, nm.length(), radix) and in the catch block the idea to throw an "nice" error can be misleading. since -02147483648 for example would become -2147483648 because the radix is 8. Since per Radix only one String is possible to get through would if not be faster and more clear to check (compare) if it is the matching string and return the correct value else throw the error. This is also easy to read and even if is on the edge avoid substring , concationation and reparsing. - PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Thu, 12 Aug 2021 21:26:08 GMT, SkateScout
wrote:
> Hi,
> i check Long.java line 1301...1311 and i am not sure if this code is really
> good.
>
> 1. If negative is false the whole part ends with two times the same
> substring and this implies the same error.
>
> 2. If negative is true and we get an error than we can throw the error
> without an second parse step or we can use the substring from the first round.
>
> 3. Also as mentioned above the parseLong(text,radix) should be changed to
> parseLong(seq, beginIndex, endIndex, radix)
>this would avoid at least in the positive case the substring at all.
>
> 4. The same points are with Integer as well.
>try {
>result = parseLong(nm.substring(index), radix);
>result = negative ? -result : result;
>} catch (NumberFormatException e) {
>// If number is Long.MIN_VALUE, we'll end up here. The next line
>// handles this case, and causes any genuine format error to be
>// rethrown.
>String constant = negative ? ("-" + nm.substring(index))
>: nm.substring(index);
>result = parseLong(constant, radix);
>}
The pre-existing logic here is iffy, but I think it is correct.
For Integer: If negative is true, then parsing "2147483648" (Integer.MAX_VALUE
+ 1) would throw, be reparsed as "-2147483648" and then be accepted as
Integer.MIN_VALUE. This is the only case that should be non-exceptional, but it
should also be exceedingly rare in practice. For negative values it improves
the error messages a bit to add the "-" and reparse.
Improving the catch clauses with `parseLong(CharSequence, int, int, int)` and
adding an `if (!negative) throw e` case to the catch could theoretically
improve performance of parsing the MIN_VALUE edge case and repeated decoding of
malformed positive numbers, but these are rare or exceptional cases where we
should favor simplicity over optimal performance
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Tue, 10 Aug 2021 21:06:00 GMT, Сергей Цыпанов
wrote:
>> The code in `Integer.decode()` and `Long.decode()` might allocate two
>> instances of Integer/Long for the negative values less than -127:
>>
>> Integer result;
>>
>> result = Integer.valueOf(nm.substring(index), radix);
>> result = negative ? Integer.valueOf(-result.intValue()) : result;
>>
>> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()`
>> method. Same applicable for `Long` and some other classes.
>
> Сергей Цыпанов has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - 8267844: Add benchmarks for Integer/Long.decode()
> - 8267844: Rid useless substring when calling Integer/Long.parse*()
> - Merge branch 'master' into 8267844
> - Merge branch 'master' into 8267844
> - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where
> applicable
Hi,
i check Long.java line 1301...1311 and i am not sure if this code is really
good.
1) If negative is false the whole part ends with two times the same substring
and this implies the same error.
2) If negative is true and we get an error than we can throw the error without
an second parse step or we can use the substring from the first round.
3) Also as mentioned above the parseLong(text,radix) should be changed to
parseLong(seq, beginIndex, endIndex, radix)
this would avoid at least in the positive case the substring at all.
4) The same points are with Integer as well.
try {
result = parseLong(nm.substring(index), radix);
result = negative ? -result : result;
} catch (NumberFormatException e) {
// If number is Long.MIN_VALUE, we'll end up here. The next line
// handles this case, and causes any genuine format error to be
// rethrown.
String constant = negative ? ("-" + nm.substring(index))
: nm.substring(index);
result = parseLong(constant, radix);
}
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Tue, 10 Aug 2021 21:06:00 GMT, Сергей Цыпанов wrote: >> The code in `Integer.decode()` and `Long.decode()` might allocate two >> instances of Integer/Long for the negative values less than -127: >> >> Integer result; >> >> result = Integer.valueOf(nm.substring(index), radix); >> result = negative ? Integer.valueOf(-result.intValue()) : result; >> >> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` >> method. Same applicable for `Long` and some other classes. > > Сергей Цыпанов has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8267844: Add benchmarks for Integer/Long.decode() > - 8267844: Rid useless substring when calling Integer/Long.parse*() > - Merge branch 'master' into 8267844 > - Merge branch 'master' into 8267844 > - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where > applicable Nice! - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
> The code in `Integer.decode()` and `Long.decode()` might allocate two > instances of Integer/Long for the negative values less than -127: > > Integer result; > > result = Integer.valueOf(nm.substring(index), radix); > result = negative ? Integer.valueOf(-result.intValue()) : result; > > To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` > method. Same applicable for `Long` and some other classes. Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - 8267844: Add benchmarks for Integer/Long.decode() - 8267844: Rid useless substring when calling Integer/Long.parse*() - Merge branch 'master' into 8267844 - Merge branch 'master' into 8267844 - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable - Changes: - all: https://git.openjdk.java.net/jdk/pull/5068/files - new: https://git.openjdk.java.net/jdk/pull/5068/files/a1b993d4..7486b13f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5068=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5068=00-01 Stats: 149574 lines in 2453 files changed: 97455 ins; 39648 del; 12471 mod Patch: https://git.openjdk.java.net/jdk/pull/5068.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5068/head:pull/5068 PR: https://git.openjdk.java.net/jdk/pull/5068
