Re: RFR: 8316704: Regex-free parsing of Formatter and FormatProcessor specifiers [v13]
On Thu, 28 Sep 2023 15:01:25 GMT, Raffaello Giulietti wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refactor according to rgiulietti's suggestion and add testcases > > Meaningful external reviews take a _lot_ of engineering time on such a > popular platform as the JDK. > They make sense only on somehow stable code, otherwise they are a waste of > human resources. > > So sure, take your time to stabilize your code, make your tests, but please > refrain from keep on changing too much once you publish a PR. If reviewers > have to face continuous changes, they might become uninterested in reviewing. @rgiulietti can you help me continue to review this PR? - PR Comment: https://git.openjdk.org/jdk/pull/15776#issuecomment-1764464777
Re: RFR: 8316704: Regex-free parsing of Formatter and FormatProcessor specifiers [v13]
On Thu, 28 Sep 2023 15:01:25 GMT, Raffaello Giulietti wrote: >> 温绍锦 has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Refactor according to rgiulietti's suggestion and add testcases > > Meaningful external reviews take a _lot_ of engineering time on such a > popular platform as the JDK. > They make sense only on somehow stable code, otherwise they are a waste of > human resources. > > So sure, take your time to stabilize your code, make your tests, but please > refrain from keep on changing too much once you publish a PR. If reviewers > have to face continuous changes, they might become uninterested in reviewing. @rgiulietti I have no plans to make changes, Can you help me continue the review? Thank you! - PR Comment: https://git.openjdk.org/jdk/pull/15776#issuecomment-1747854315
Re: RFR: 8316704: Regex-free parsing of Formatter and FormatProcessor specifiers [v13]
On Wed, 27 Sep 2023 21:26:46 GMT, 温绍锦 wrote: >> @cl4es made performance optimizations for the simple specifiers of >> String.format in PR https://github.com/openjdk/jdk/pull/2830. Based on the >> same idea, I continued to make improvements. I made patterns like %2d %02d >> also be optimized. >> >> The following are the test results based on MacBookPro M1 Pro: >> >> >> -Benchmark Mode Cnt Score Error Units >> -StringFormat.complexFormat avgt 15 1862.233 ? 217.479 ns/op >> -StringFormat.int02Format avgt 15 312.491 ? 26.021 ns/op >> -StringFormat.intFormat avgt 1584.432 ? 4.145 ns/op >> -StringFormat.longFormatavgt 1587.330 ? 6.111 ns/op >> -StringFormat.stringFormat avgt 1563.985 ? 11.366 ns/op >> -StringFormat.stringIntFormat avgt 1587.422 ? 0.147 ns/op >> -StringFormat.widthStringFormat avgt 15 250.740 ? 32.639 ns/op >> -StringFormat.widthStringIntFormat avgt 15 312.474 ? 16.309 ns/op >> >> +Benchmark Mode CntScoreError Units >> +StringFormat.complexFormat avgt 15 740.626 ? 66.671 ns/op >> (+151.45) >> +StringFormat.int02Format avgt 15 131.049 ? 0.432 ns/op >> (+138.46) >> +StringFormat.intFormat avgt 15 67.229 ? 4.155 ns/op >> (+25.59) >> +StringFormat.longFormatavgt 15 66.444 ? 0.614 ns/op >> (+31.44) >> +StringFormat.stringFormat avgt 15 62.619 ? 4.652 ns/op >> (+2.19) >> +StringFormat.stringIntFormat avgt 15 89.606 ? 13.966 ns/op >> (-2.44) >> +StringFormat.widthStringFormat avgt 15 52.462 ? 15.649 ns/op >> (+377.95) >> +StringFormat.widthStringIntFormat avgt 15 101.814 ? 3.147 ns/op >> (+206.91) > > 温绍锦 has updated the pull request incrementally with one additional commit > since the last revision: > > Refactor according to rgiulietti's suggestion and add testcases Meaningful external reviews take a _lot_ of engineering time on such a popular platform as the JDK. They make sense only on somehow stable code, otherwise they are a waste of human resources. So sure, take your time to stabilize your code, make your tests, but please refrain from keep on changing too much once you publish a PR. If reviewers have to face continuous changes, they might become uninterested in reviewing. - PR Comment: https://git.openjdk.org/jdk/pull/15776#issuecomment-1739462969
Re: RFR: 8316704: Regex-free parsing of Formatter and FormatProcessor specifiers [v13]
On Wed, 27 Sep 2023 21:26:46 GMT, 温绍锦 wrote:
>> @cl4es made performance optimizations for the simple specifiers of
>> String.format in PR https://github.com/openjdk/jdk/pull/2830. Based on the
>> same idea, I continued to make improvements. I made patterns like %2d %02d
>> also be optimized.
>>
>> The following are the test results based on MacBookPro M1 Pro:
>>
>>
>> -Benchmark Mode Cnt Score Error Units
>> -StringFormat.complexFormat avgt 15 1862.233 ? 217.479 ns/op
>> -StringFormat.int02Format avgt 15 312.491 ? 26.021 ns/op
>> -StringFormat.intFormat avgt 1584.432 ? 4.145 ns/op
>> -StringFormat.longFormatavgt 1587.330 ? 6.111 ns/op
>> -StringFormat.stringFormat avgt 1563.985 ? 11.366 ns/op
>> -StringFormat.stringIntFormat avgt 1587.422 ? 0.147 ns/op
>> -StringFormat.widthStringFormat avgt 15 250.740 ? 32.639 ns/op
>> -StringFormat.widthStringIntFormat avgt 15 312.474 ? 16.309 ns/op
>>
>> +Benchmark Mode CntScoreError Units
>> +StringFormat.complexFormat avgt 15 740.626 ? 66.671 ns/op
>> (+151.45)
>> +StringFormat.int02Format avgt 15 131.049 ? 0.432 ns/op
>> (+138.46)
>> +StringFormat.intFormat avgt 15 67.229 ? 4.155 ns/op
>> (+25.59)
>> +StringFormat.longFormatavgt 15 66.444 ? 0.614 ns/op
>> (+31.44)
>> +StringFormat.stringFormat avgt 15 62.619 ? 4.652 ns/op
>> (+2.19)
>> +StringFormat.stringIntFormat avgt 15 89.606 ? 13.966 ns/op
>> (-2.44)
>> +StringFormat.widthStringFormat avgt 15 52.462 ? 15.649 ns/op
>> (+377.95)
>> +StringFormat.widthStringIntFormat avgt 15 101.814 ? 3.147 ns/op
>> (+206.91)
>
> 温绍锦 has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Refactor according to rgiulietti's suggestion and add testcases
For maintainability purposes, we should strive for simplicity and readability.
src/java.base/share/classes/java/util/Formatter.java line 2899:
> 2897: if (c == '.') {
> 2898: // (\.\d+)?
> 2899: precisionSize = parsePrecision();
`precisionSize == 0` has two different meanings: (1) there's a '.' not followed
by digits and (2) there's no precision field at all.
In the proposed
[alternative](https://github.com/openjdk/jdk/pull/15776#issuecomment-1737986458),
case (1) is indicated by `-1`, while case (2) is indicated by `0`. Besides,
the decision is taken in the `parsePrecision()` method, not partly here and
partly in the method.
src/java.base/share/classes/java/util/Formatter.java line 2934:
> 2932: // (\d+\$)?
> 2933: int i = off;
> 2934: for (; i < max && isDigit(c); c = next(++i)); // empty body
No need for `next()` if done as in the proposed alternative.
src/java.base/share/classes/java/util/Formatter.java line 2947:
> 2945: c = first;
> 2946: }
> 2947: }
I think the logic in the proposed alternative is more readable.
src/java.base/share/classes/java/util/Formatter.java line 2953:
> 2951: // ([-#+ 0,(\<]*)?
> 2952: int i = off;
> 2953: for (; i < max && Flags.isFlag(c); c = next(++i)); //
> empty body
No need for `next()` if done as in the proposed alternative.
src/java.base/share/classes/java/util/Formatter.java line 2961:
> 2959: // (\d+)?
> 2960: int i = off;
> 2961: for (; i < max && isDigit(c); c = next(++i)); // empty body
You are using `next()` here, but the simpler solution in `parsePrecision()`.
I think consistency simplifies understanding.
src/java.base/share/classes/java/util/Formatter.java line 2978:
> 2976: }
> 2977:
> 2978: private char next(int off) {
There's no real need for this more expensive method, compared with a simple
`s.charAt(i)`, in the alternative proposed
[before](https://github.com/openjdk/jdk/pull/15776#issuecomment-1737986458).
-
PR Review: https://git.openjdk.org/jdk/pull/15776#pullrequestreview-1647449239
PR Review Comment: https://git.openjdk.org/jdk/pull/15776#discussion_r1339852931
PR Review Comment: https://git.openjdk.org/jdk/pull/15776#discussion_r1339856693
PR Review Comment: https://git.openjdk.org/jdk/pull/15776#discussion_r1339856725
PR Review Comment: https://git.openjdk.org/jdk/pull/15776#discussion_r1339857403
PR Review Comment: https://git.openjdk.org/jdk/pull/15776#discussion_r1339844252
PR Review Comment: https://git.openjdk.org/jdk/pull/15776#discussion_r1339842414
Re: RFR: 8316704: Regex-free parsing of Formatter and FormatProcessor specifiers [v13]
> @cl4es made performance optimizations for the simple specifiers of > String.format in PR https://github.com/openjdk/jdk/pull/2830. Based on the > same idea, I continued to make improvements. I made patterns like %2d %02d > also be optimized. > > The following are the test results based on MacBookPro M1 Pro: > > > -Benchmark Mode Cnt Score Error Units > -StringFormat.complexFormat avgt 15 1862.233 ? 217.479 ns/op > -StringFormat.int02Format avgt 15 312.491 ? 26.021 ns/op > -StringFormat.intFormat avgt 1584.432 ? 4.145 ns/op > -StringFormat.longFormatavgt 1587.330 ? 6.111 ns/op > -StringFormat.stringFormat avgt 1563.985 ? 11.366 ns/op > -StringFormat.stringIntFormat avgt 1587.422 ? 0.147 ns/op > -StringFormat.widthStringFormat avgt 15 250.740 ? 32.639 ns/op > -StringFormat.widthStringIntFormat avgt 15 312.474 ? 16.309 ns/op > > +Benchmark Mode CntScoreError Units > +StringFormat.complexFormat avgt 15 740.626 ? 66.671 ns/op > (+151.45) > +StringFormat.int02Format avgt 15 131.049 ? 0.432 ns/op > (+138.46) > +StringFormat.intFormat avgt 15 67.229 ? 4.155 ns/op > (+25.59) > +StringFormat.longFormatavgt 15 66.444 ? 0.614 ns/op > (+31.44) > +StringFormat.stringFormat avgt 15 62.619 ? 4.652 ns/op (+2.19) > +StringFormat.stringIntFormat avgt 15 89.606 ? 13.966 ns/op (-2.44) > +StringFormat.widthStringFormat avgt 15 52.462 ? 15.649 ns/op > (+377.95) > +StringFormat.widthStringIntFormat avgt 15 101.814 ? 3.147 ns/op > (+206.91) 温绍锦 has updated the pull request incrementally with one additional commit since the last revision: Refactor according to rgiulietti's suggestion and add testcases - Changes: - all: https://git.openjdk.org/jdk/pull/15776/files - new: https://git.openjdk.org/jdk/pull/15776/files/8a6fe0e9..3ff5121a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15776=12 - incr: https://webrevs.openjdk.org/?repo=jdk=15776=11-12 Stats: 47 lines in 2 files changed: 10 ins; 14 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/15776.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15776/head:pull/15776 PR: https://git.openjdk.org/jdk/pull/15776
