Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v3]
On Tue, 5 Mar 2024 21:27:09 GMT, Justin Lu wrote: >> Please review this PR and corresponding CSR which prevents an >> OutOfMemoryError by restricting the initial maximum fraction digits for an >> empty pattern DecimalFormat. >> >> For an empty String pattern DecimalFormat, the maximum fraction digits is >> initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, >> StringBuilder internally doubles capacity attempting to append >> Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral >> compatibility changes. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > reflect review LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18094#pullrequestreview-1918175848
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v4]
> Please review this PR and corresponding CSR which prevents an > OutOfMemoryError by restricting the initial maximum fraction digits for an > empty pattern DecimalFormat. > > For an empty String pattern DecimalFormat, the maximum fraction digits is > initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, > StringBuilder internally doubles capacity attempting to append > Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral > compatibility changes. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: locale no longer needed, remove unusued imports - Changes: - all: https://git.openjdk.org/jdk/pull/18094/files - new: https://git.openjdk.org/jdk/pull/18094/files/7ee87fc2..e9e86da7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18094&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18094&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18094.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18094/head:pull/18094 PR: https://git.openjdk.org/jdk/pull/18094
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]
On Tue, 5 Mar 2024 21:23:48 GMT, Justin Lu wrote:
>> test/jdk/java/text/Format/DecimalFormat/ToPatternTest.java line 60:
>>
>>> 58: dFmt.setMinimumFractionDigits(minFrac);
>>> 59:
>>> 60: String[] patterns = dFmt.toPattern().split("\\.");
>>
>> Instead of assuming/hardcoding the decimal separator,
>> `DecimalFormatSymbols.getInstance(Locale.US).getDecimalSeparator()` may be
>> better.
>
> Actually, since `toPattern()` is non-localized, it will always use "." as
> specified by DecimalFormat.
>
> I removed the misleading comment `// Use a US dFmt, which uses '.' as the
> decimal separator` that indicated we needed to use a DecimalFormatSymbols
> where "." was the decimal separator. Can just simply use a DecimalFormat
> returned by the default constructor.
Oh right. Yes, so we should not use the US locale for this test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513526760
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]
On Tue, 5 Mar 2024 19:10:19 GMT, Naoto Sato wrote:
>> Justin Lu 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:
>>
>> - cleanup/typo
>> - Merge branch 'master' into JDK-8326908-emptyPattern-OOME
>> - implement feedback + improve pattern related tests
>> - minor additions
>> - init
>
> test/jdk/java/text/Format/DecimalFormat/ToPatternTest.java line 60:
>
>> 58: dFmt.setMinimumFractionDigits(minFrac);
>> 59:
>> 60: String[] patterns = dFmt.toPattern().split("\\.");
>
> Instead of assuming/hardcoding the decimal separator,
> `DecimalFormatSymbols.getInstance(Locale.US).getDecimalSeparator()` may be
> better.
Actually, since `toPattern()` is non-localized, it will always use "." as
specified by DecimalFormat.
I removed the misleading comment `// Use a US dFmt, which uses '.' as the
decimal separator` that indicated we needed to use a DecimalFormatSymbols where
"." was the decimal separator. Can just simply use a DecimalFormat returned by
the default constructor.
> test/jdk/java/text/Format/DecimalFormat/ToPatternTest.java line 110:
>
>> 108: // 8326908: Verify that an empty pattern DecimalFormat does not
>> throw an
>> 109: // OutOfMemoryError when toPattern() is invoked. Behavioral change
>> of
>> 110: // MAXIMUM_INTEGER_DIGITS replaced with DOUBLE_FRACTION_DIGITS for
>> empty
>
> Should we explicitly check `new DecimalFormat("").getMaximumFractionDigits()
> == DOUBLE_FRACTION_DIGITS`?
Thanks for the review, added an explicit check
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513517348
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513517336
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v3]
> Please review this PR and corresponding CSR which prevents an > OutOfMemoryError by restricting the initial maximum fraction digits for an > empty pattern DecimalFormat. > > For an empty String pattern DecimalFormat, the maximum fraction digits is > initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, > StringBuilder internally doubles capacity attempting to append > Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral > compatibility changes. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: reflect review - Changes: - all: https://git.openjdk.org/jdk/pull/18094/files - new: https://git.openjdk.org/jdk/pull/18094/files/7ca56500..7ee87fc2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18094&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18094&range=01-02 Stats: 20 lines in 2 files changed: 8 ins; 4 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18094.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18094/head:pull/18094 PR: https://git.openjdk.org/jdk/pull/18094
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]
On Tue, 5 Mar 2024 00:32:47 GMT, Justin Lu wrote:
>> Please review this PR and corresponding CSR which prevents an
>> OutOfMemoryError by restricting the initial maximum fraction digits for an
>> empty pattern DecimalFormat.
>>
>> For an empty String pattern DecimalFormat, the maximum fraction digits is
>> initialized to `Integer.MAX_VALUE`. When toPattern() is invoked,
>> StringBuilder internally doubles capacity attempting to append
>> Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral
>> compatibility changes.
>
> Justin Lu 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:
>
> - cleanup/typo
> - Merge branch 'master' into JDK-8326908-emptyPattern-OOME
> - implement feedback + improve pattern related tests
> - minor additions
> - init
src/java.base/share/classes/java/text/DecimalFormat.java line 3367:
> 3365: }
> 3366: }
> 3367: return false;
can simply be a single return statement.
test/jdk/java/text/Format/DecimalFormat/ToPatternTest.java line 60:
> 58: dFmt.setMinimumFractionDigits(minFrac);
> 59:
> 60: String[] patterns = dFmt.toPattern().split("\\.");
Instead of assuming/hardcoding the decimal separator,
`DecimalFormatSymbols.getInstance(Locale.US).getDecimalSeparator()` may be
better.
test/jdk/java/text/Format/DecimalFormat/ToPatternTest.java line 110:
> 108: // 8326908: Verify that an empty pattern DecimalFormat does not
> throw an
> 109: // OutOfMemoryError when toPattern() is invoked. Behavioral change of
> 110: // MAXIMUM_INTEGER_DIGITS replaced with DOUBLE_FRACTION_DIGITS for
> empty
Should we explicitly check `new DecimalFormat("").getMaximumFractionDigits() ==
DOUBLE_FRACTION_DIGITS`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513356393
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513364152
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513378668
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
On Tue, 5 Mar 2024 00:15:10 GMT, Justin Lu wrote: > For clarification, this is entirely a bug with DecimalFormat, not > StringBuilder. An empty String pattern DecimalFormat sets the maximum > fraction digits to Integer.MAX_VALUE. When toPattern() is invoked, the local > StringBuilder will append until an OOME is thrown by the StringBuilder as > there is not enough memory, when internally, the buffer is doubled for a > value too large. But such an OOME would occur for any large enough value, so > the issue lies with DecimalFormat. Got it. Thanks for your explanation. - PR Comment: https://git.openjdk.org/jdk/pull/18094#issuecomment-1978591032
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
On Sun, 3 Mar 2024 05:00:36 GMT, Guoxiong Li wrote: >> Please review this PR and corresponding CSR which prevents an >> OutOfMemoryError by restricting the initial maximum fraction digits for an >> empty pattern DecimalFormat. >> >> For an empty String pattern DecimalFormat, the maximum fraction digits is >> initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, >> StringBuilder internally doubles capacity attempting to append >> Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral >> compatibility changes. > >> When toPattern() is invoked, StringBuilder internally doubles capacity >> attempting to append Integer.MAX_VALUE digits until OOME occurs. > > It seems a bug in `toPattern` or `StringBuilder`? May be better to > investigate more about it. Hi @lgxbslgx, For clarification, this is entirely a bug with DecimalFormat, not StringBuilder. An empty String pattern DecimalFormat sets the maximum fraction digits to `Integer.MAX_VALUE`. When toPattern() is invoked, the local StringBuilder will append until an OOME is thrown by the StringBuilder as there is not enough memory, when internally, the buffer is doubled for a value too large. But such an OOME would occur for any large enough value, so the issue lies with DecimalFormat. - PR Comment: https://git.openjdk.org/jdk/pull/18094#issuecomment-1977712428
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]
On Mon, 4 Mar 2024 16:03:55 GMT, Roger Riggs wrote: >> Justin Lu 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: >> >> - cleanup/typo >> - Merge branch 'master' into JDK-8326908-emptyPattern-OOME >> - implement feedback + improve pattern related tests >> - minor additions >> - init > > src/java.base/share/classes/java/text/DecimalFormat.java line 3717: > >> 3715: // As maxFracDigits are fully displayed unlike maxIntDigits >> 3716: // Prevent OOME by setting to a much more reasonable value. >> 3717: setMaximumFractionDigits(DOUBLE_FRACTION_DIGITS); > > Setting a reasonable default makes sense. > In other control paths, the max fraction digits come from the inputs or are > explicitly set. > > It might be a reasonable related change to use StringBuilder.repeat() instead > of a loop at LIne 3312-3319, where the pattern char(s) are being appended to > the result. Thanks for taking a look. Updated the loop with `repeat` as you suggested, and decided to refactor the rest of the method while I was at it. Additionally, I added some more tests, as it seems that there is a lack of pattern tests for DecimalFormat in general. > In other control paths, the max fraction digits come from the inputs or are > explicitly set. Right, while `Integer.MAX_VALUE` can still be set by other control paths (and thus an OOME if toPattern() is invoked), this is something explicitly done by the user, and thus we decided we would still allow the behavior. - PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1511966791
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]
> Please review this PR and corresponding CSR which prevents an > OutOfMemoryError by restricting the initial maximum fraction digits for an > empty pattern DecimalFormat. > > For an empty String pattern DecimalFormat, the maximum fraction digits is > initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, > StringBuilder internally doubles capacity attempting to append > Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral > compatibility changes. Justin Lu 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: - cleanup/typo - Merge branch 'master' into JDK-8326908-emptyPattern-OOME - implement feedback + improve pattern related tests - minor additions - init - Changes: - all: https://git.openjdk.org/jdk/pull/18094/files - new: https://git.openjdk.org/jdk/pull/18094/files/fc8a7ce4..7ca56500 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18094&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18094&range=00-01 Stats: 5885 lines in 919 files changed: 2483 ins; 1248 del; 2154 mod Patch: https://git.openjdk.org/jdk/pull/18094.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18094/head:pull/18094 PR: https://git.openjdk.org/jdk/pull/18094
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
On Sat, 2 Mar 2024 00:34:32 GMT, Justin Lu wrote: > Please review this PR and corresponding CSR which prevents an > OutOfMemoryError by restricting the initial maximum fraction digits for an > empty pattern DecimalFormat. > > For an empty String pattern DecimalFormat, the maximum fraction digits is > initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, > StringBuilder internally doubles capacity attempting to append > Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral > compatibility changes. Would declaring an object for the input and then invoking the two methods on the same object rather than creating two objects be considered an alternative? - Marked as reviewed by rajuc...@github.com (no known OpenJDK username). PR Review: https://git.openjdk.org/jdk/pull/18094#pullrequestreview-1915406613
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
On Sat, 2 Mar 2024 00:34:32 GMT, Justin Lu wrote: > Please review this PR and corresponding CSR which prevents an > OutOfMemoryError by restricting the initial maximum fraction digits for an > empty pattern DecimalFormat. > > For an empty String pattern DecimalFormat, the maximum fraction digits is > initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, > StringBuilder internally doubles capacity attempting to append > Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral > compatibility changes. src/java.base/share/classes/java/text/DecimalFormat.java line 3717: > 3715: // As maxFracDigits are fully displayed unlike maxIntDigits > 3716: // Prevent OOME by setting to a much more reasonable value. > 3717: setMaximumFractionDigits(DOUBLE_FRACTION_DIGITS); Setting a reasonable default makes sense. In other control paths, the max fraction digits come from the inputs or are explicitly set. It might be a reasonable related change to use StringBuilder.repeat() instead of a loop at LIne 3312-3319, where the pattern char(s) are being appended to the result. - PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1511404338
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
On Sat, 2 Mar 2024 00:34:32 GMT, Justin Lu wrote: > When toPattern() is invoked, StringBuilder internally doubles capacity > attempting to append Integer.MAX_VALUE digits until OOME occurs. It seems a bug in `toPattern` or `StringBuilder`? May be better to investigate more about it. - PR Comment: https://git.openjdk.org/jdk/pull/18094#issuecomment-1975023075
RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
Please review this PR and corresponding CSR which prevents an OutOfMemoryError by restricting the initial maximum fraction digits for an empty pattern DecimalFormat. The cause is that the maximum fraction digits is initialized to `Integer.MAX_VALUE` which causes StringBuilder to eventually throw OOME when internally doubling capacity too many times when toPattern is invoked. CSR covers potential behavioral compatibility changes. - Commit messages: - minor additions - init Changes: https://git.openjdk.org/jdk/pull/18094/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18094&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326908 Stats: 52 lines in 2 files changed: 51 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18094.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18094/head:pull/18094 PR: https://git.openjdk.org/jdk/pull/18094
