Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v4]
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Sergey Bylokhov has updated the pull request incrementally with one additional
commit since the last revision:
Update IPPPrintService.java
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4951/files
- new: https://git.openjdk.java.net/jdk/pull/4951/files/e4e82c83..47ed1e81
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4951=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4951=02-03
Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/4951.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4951/head:pull/4951
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v3]
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Sergey Bylokhov has updated the pull request incrementally with one additional
commit since the last revision:
static import StandardCharsets
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4951/files
- new: https://git.openjdk.java.net/jdk/pull/4951/files/886264aa..e4e82c83
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4951=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4951=01-02
Stats: 245 lines in 31 files changed: 93 ins; 63 del; 89 mod
Patch: https://git.openjdk.java.net/jdk/pull/4951.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4951/head:pull/4951
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 22:03:54 GMT, Sergey Bylokhov wrote: > > I am fine to do that, if there are no objections I can change the whole fix. Modifying the entire changeset seems like an overkill. Using static imports in only one file is _inconsistent_, yet it makes the places where the encodings are used clearer. - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 21:54:08 GMT, Alexey Ivanov wrote: >> it is aligned already, the StandardCharsets.UTF_8 is parameter of "new >> String()", not the getTransferData. > > Ah, right! > But it's confusing: it looks as if `StandardCharsets.UTF_8` is a parameter to > `getTransferData`. Maybe avoid breaking the line and leave `UTF_8` on the > same line? If you import `UTF_8` and `UTF_16LE` statically, line break is > unnecessary. I am fine to do that, if there are no objections I can change the whole fix. - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 21:18:40 GMT, Alexey Ivanov wrote: >> Sergey Bylokhov 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 two additional >> commits since the last revision: >> >> - Merge branch 'master' into JDK-8271456 >> - Initial fix for JDK-8271456 > > src/java.desktop/windows/classes/sun/awt/windows/WDataTransferer.java line > 270: > >> 268: charset = new String((byte[])localeTransferable. >> 269: getTransferData(javaTextEncodingFlavor), >> 270: StandardCharsets.UTF_8); > > Suggestion: > > getTransferData(javaTextEncodingFlavor), > StandardCharsets.UTF_8); > > The parameter on the second line should probably be aligned with the first > parameter as it's done in the snippet above. it is aligned already, the StandardCharsets.UTF_8 is parameter of "new String()", not the getTransferData. - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 19:37:04 GMT, Sergey Bylokhov wrote:
>> This is a request to clean up a desktop module as was done in JDK-8233884
>> for "java.base" module.
>>
>> In many places standard charsets are looked up via their names, for example:
>> absolutePath.getBytes("UTF-8");
>>
>> This could be done more efficiently(x20 time faster) with use of
>> java.nio.charset.StandardCharsets:
>> absolutePath.getBytes(StandardCharsets.UTF_8);
>>
>> The later variant also makes the code cleaner, as it is known not to throw
>> UnsupportedEncodingException in contrary to the former variant.
>>
>> Tested by the desktop headless/headful tests on linux/windows.
>
> Sergey Bylokhov 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 two additional
> commits since the last revision:
>
> - Merge branch 'master' into JDK-8271456
> - Initial fix for JDK-8271456
Marked as reviewed by aivanov (Reviewer).
src/java.desktop/windows/classes/sun/awt/windows/WDataTransferer.java line 270:
> 268: charset = new String((byte[])localeTransferable.
> 269: getTransferData(javaTextEncodingFlavor),
> 270: StandardCharsets.UTF_8);
Suggestion:
getTransferData(javaTextEncodingFlavor),
StandardCharsets.UTF_8);
The parameter on the second line should probably be aligned with the first
parameter as it's done in the snippet above.
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL
On Tue, 3 Aug 2021 19:53:22 GMT, Sergey Bylokhov wrote: >> Looks like making MTLLayer opaque by default require much more time that I >> expected. I tried different things to resolve artefacts that I mentioned >> before but nothing seems to work. So, I suggest to stick with my latest >> approach. It passes all the tests and don't break SwingSet2. >> >>> BTW Probably implementation of LWWindowPeer.setTextured() should call the >>> updateOpaque() as well. >> >> I'll add this change > > How to reproduce that artifact? 1. Run SwingSet2 2. Click on all tabs starting from the second one 3. On the tab with swing tree control try to expand some tree nodes -> you'll see black rectangles while performing the clicks It may not be reproduced on a first attempt. On my machine it's reproducible with probability at least 50% - PR: https://git.openjdk.java.net/jdk/pull/4946
Re: [OpenJDK 2D-Dev] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL
On Tue, 3 Aug 2021 06:46:07 GMT, Alexey Ushakov wrote: >> I need to look at it closely, I do not understand why we made the native >> layer opaque=NO by default while the java code uses opaque=true by default. >> And then we reset the native code from NO to yes by the code above. Why we >> cannot make both native/java use true/YES and when necessary it will be >> reset to false/NO by the existing code(via LWWindowPeer.setOpaque() and >> LWWindowPeer.updateOpaque?) >> BTW Probably implementation of LWWindowPeer.setTextured() should call the >> updateOpaque() as well. > > Yes, sounds reasonable. Moreover, It was my initial attempt but I faced with > some regressions in SwingSet2 demo. In some cases background of tree nodes > become black and this artifact was reproducible only if some other tabs were > clicked before. It was hot time for jdk17 so I decided not to fight with > this regression. Now we have more time, so I can spend more time on this. Looks like making MTLLayer opaque by default require much more time that I expected. I tried different things to resolve artefacts that I mentioned before but nothing seems to work. So, I suggest to stick with my latest approach. It passes all the tests and don't break SwingSet2. > BTW Probably implementation of LWWindowPeer.setTextured() should call the > updateOpaque() as well. I'll add this change - PR: https://git.openjdk.java.net/jdk/pull/4946
Re: [OpenJDK 2D-Dev] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL
On Tue, 3 Aug 2021 19:51:30 GMT, Alexey Ushakov wrote: >> Yes, sounds reasonable. Moreover, It was my initial attempt but I faced with >> some regressions in SwingSet2 demo. In some cases background of tree nodes >> become black and this artifact was reproducible only if some other tabs were >> clicked before. It was hot time for jdk17 so I decided not to fight with >> this regression. Now we have more time, so I can spend more time on this. > > Looks like making MTLLayer opaque by default require much more time that I > expected. I tried different things to resolve artefacts that I mentioned > before but nothing seems to work. So, I suggest to stick with my latest > approach. It passes all the tests and don't break SwingSet2. > >> BTW Probably implementation of LWWindowPeer.setTextured() should call the >> updateOpaque() as well. > > I'll add this change How to reproduce that artifact? - PR: https://git.openjdk.java.net/jdk/pull/4946
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Sergey Bylokhov 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 two additional
commits since the last revision:
- Merge branch 'master' into JDK-8271456
- Initial fix for JDK-8271456
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4951/files
- new: https://git.openjdk.java.net/jdk/pull/4951/files/430c9b3a..886264aa
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4951=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4951=00-01
Stats: 1774 lines in 83 files changed: 1230 ins; 337 del; 207 mod
Patch: https://git.openjdk.java.net/jdk/pull/4951.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4951/head:pull/4951
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8270893: IndexOutOfBoundsException while reading large TIFF file [v2]
On Tue, 3 Aug 2021 09:11:07 GMT, Jayathirth D V wrote: >> We are incorrectly passing source offset to ImageInputStream.readFully() >> which is getting used on destination buffer. streamPos maintained in each >> implementation of stream maintain's appropriate source offset while reading >> the data. Since we are completely utilizing destination buffer any offset >> greater than 0 would cause IOOBE. In our case we should use 0 as offset >> value. >> >> Also to hit this code we need stream/file with at-least 1MB of IFD data, >> that's why there is no regression test. This change can be verified using >> image attached in JBS. All test run is green. > > Jayathirth D V has updated the pull request incrementally with one additional > commit since the last revision: > > Added regression test Thank you! look fine. BTW Looks like stream.readFully(unit, 0, sz) can be simplified to the stream.readFully(unit)? - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4836
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
On Sun, 1 Aug 2021 07:07:21 GMT, Sergey Bylokhov wrote:
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Marked as reviewed by azvegint (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
On Tue, 3 Aug 2021 13:30:28 GMT, Alexander Zvegintsev
wrote:
>> This is a request to clean up a desktop module as was done in JDK-8233884
>> for "java.base" module.
>>
>> In many places standard charsets are looked up via their names, for example:
>> absolutePath.getBytes("UTF-8");
>>
>> This could be done more efficiently(x20 time faster) with use of
>> java.nio.charset.StandardCharsets:
>> absolutePath.getBytes(StandardCharsets.UTF_8);
>>
>> The later variant also makes the code cleaner, as it is known not to throw
>> UnsupportedEncodingException in contrary to the former variant.
>>
>> Tested by the desktop headless/headful tests on linux/windows.
>
> src/java.desktop/share/classes/com/sun/imageio/plugins/wbmp/WBMPMetadata.java
> line 25:
>
>> 23: * questions.
>> 24: */
>> 25:
>
> Since you updating a copyright year in all other files, you probably might
> want to update in this file too.
I did not update it since this is the only file where the imports are updated
only(the UnsupportedEncodingException is removed).
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
On Sun, 1 Aug 2021 07:07:21 GMT, Sergey Bylokhov wrote:
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
src/java.desktop/share/classes/com/sun/imageio/plugins/wbmp/WBMPMetadata.java
line 25:
> 23: * questions.
> 24: */
> 25:
Since you updating a copyright year in all other files, you probably might want
to update in this file too.
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
On Sun, 1 Aug 2021 07:07:21 GMT, Sergey Bylokhov wrote:
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Marked as reviewed by jdv (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: [OpenJDK 2D-Dev] RFR: 8270893: IndexOutOfBoundsException while reading large TIFF file [v2]
> We are incorrectly passing source offset to ImageInputStream.readFully() > which is getting used on destination buffer. streamPos maintained in each > implementation of stream maintain's appropriate source offset while reading > the data. Since we are completely utilizing destination buffer any offset > greater than 0 would cause IOOBE. In our case we should use 0 as offset value. > > Also to hit this code we need stream/file with at-least 1MB of IFD data, > that's why there is no regression test. This change can be verified using > image attached in JBS. All test run is green. Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision: Added regression test - Changes: - all: https://git.openjdk.java.net/jdk/pull/4836/files - new: https://git.openjdk.java.net/jdk/pull/4836/files/81d54f09..9c7e113c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4836=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4836=00-01 Stats: 76 lines in 1 file changed: 76 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4836.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4836/head:pull/4836 PR: https://git.openjdk.java.net/jdk/pull/4836
Re: [OpenJDK 2D-Dev] RFR: 8270893: IndexOutOfBoundsException while reading large TIFF file
On Sat, 24 Jul 2021 03:17:11 GMT, Sergey Bylokhov wrote: >> We are incorrectly passing source offset to ImageInputStream.readFully() >> which is getting used on destination buffer. streamPos maintained in each >> implementation of stream maintain's appropriate source offset while reading >> the data. Since we are completely utilizing destination buffer any offset >> greater than 0 would cause IOOBE. In our case we should use 0 as offset >> value. >> >> Also to hit this code we need stream/file with at-least 1MB of IFD data, >> that's why there is no regression test. This change can be verified using >> image attached in JBS. All test run is green. > > 1024000 bytes does not look like a big problem especially if it is generated > on the fly during the test execution? > You can try to load the data for our PYCC color profile and set one of its > tag data to a big chunk like icSigCopyrightTag, or you can try to just fill > it by zeros at the end. @mrserb added regression test, please review. - PR: https://git.openjdk.java.net/jdk/pull/4836
Re: [OpenJDK 2D-Dev] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL
On Mon, 2 Aug 2021 21:53:02 GMT, Sergey Bylokhov wrote: >> Yes, and we need this call here to initialise content view with correct >> default value >> CPlatformWindow.java:931 contentView.setWindowLayerOpaque(isOpaque); > > I need to look at it closely, I do not understand why we made the native > layer opaque=NO by default while the java code uses opaque=true by default. > And then we reset the native code from NO to yes by the code above. Why we > cannot make both native/java use true/YES and when necessary it will be reset > to false/NO by the existing code(via LWWindowPeer.setOpaque() and > LWWindowPeer.updateOpaque?) > BTW Probably implementation of LWWindowPeer.setTextured() should call the > updateOpaque() as well. Yes, sounds reasonable. Moreover, It was my initial attempt but I faced with some regressions in SwingSet2 demo. In some cases background of tree nodes become black and this artifact was reproducible only if some other tabs were clicked before. It was hot time for jdk17 so I decided not to fight with this regression. Now we have more time, so I can spend more time on this. - PR: https://git.openjdk.java.net/jdk/pull/4946
