Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons
On Wed, 10 Mar 2021 00:55:00 GMT, Sergey Bylokhov wrote: >> Continuation of review at https://github.com/openjdk/jdk/pull/380 > > Just to continue the thread from the old review, did you have a chance to > look at the possibility of loading all existed icons embedded in the file? It > was discussed here: > https://github.com/openjdk/jdk/pull/380#issuecomment-702999573 > Just to continue the thread from the old review, did you have a chance to > look at the possibility of loading all existed icons embedded in the file? It > was discussed here: > [#380 > (comment)](https://github.com/openjdk/jdk/pull/380#issuecomment-702999573) Yes, i did. There were 3 shortcomings: 1. It works in about 10% of the cases in all other cases it reports only one icon available (32x32) even if there are other resolution icons. 2. It is quite slow, especially on network drives. 3. Does not work at all in files that are within AppData\Roaming folder - returning list is just empty. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8262470: Printed GlyphVector outline with low DPI has bad quality on Windows
On Wed, 10 Mar 2021 01:48:28 GMT, Sergey Bylokhov wrote: >> Printing text using GlyphVector outline has bad quality on printers with low >> DPI on Windows. >> The GDI system used for text printing on Windows accepts only integer path >> coordinates. >> Rounding GlyphVector outline coordinates leads to distorted printed text. >> >> The issue had been reported as JDK-8256264 but was reverted because of the >> regression JDK-8259007 "This test printed a blank page". >> >> The fix JDK-8256264 scaled coordinates in wPrinterJob.moveTo()/lineTo() >> methods up and scaled transforms in wPrinterJob.beginPath()/endPath() down. >> >> The regression was in the WPathGraphics.deviceDrawLine() method which uses >> wPrinterJob.moveTo()/lineTo() methods without surrounding them with >> wPrinterJob.beginPath()/endPath() so the line coordinates were only scaled >> up. >> >> I tried to put wPrinterJob.beginPath()/endPath() methods around >> wPrinterJob.moveTo()/lineTo() in the method WPathGraphics.deviceDrawLine() >> but the line was not drawn at all even without scaling coordinates up and >> transform down (without JDK-8256264 fix). It looks like GDI treats this case >> as an empty shape. >> >> The proposed fix applies path coordinates and transform scaling only in >> WPathGraphics.convertToWPath() method. >> The one more PathPrecisionScaleFactorShapeTest.java manual test is added >> which checks that all methods that draw paths in WPathGraphics are used: >> line in WPathGraphics.deviceDrawLine() and SEG_LINETO/SEG_QUADTO/SEG_CUBICTO >> in WPathGraphics.convertToWPath() . >> >> The `java/awt/print` and `java/awt/PrintJob` automatic and manual tests were >> run on Windows 10 Pro with the fix. >> >> There are two failed automated tests which fail without the fix as well: >> java/awt/print/PrinterJob/GlyphPositions.java >> java/awt/print/PrinterJob/PSQuestionMark.java >> >> The following manual tests have issues on my system: >> - `java/awt/print/Dialog/PrintDlgPageable.java` >> java.lang.IllegalAccessException: class >> com.sun.javatest.regtest.agent.MainWrapper$MainThread cannot access a member >> of class PrintDlgPageable with modifiers "public static" >> >> - `java/awt/print/PrinterJob/PrintAttributeUpdateTest.java` I select pages >> radio button, press the print button but the test does not finish and I do >> not see any other dialogs with pass/fail buttons. >> >> - `java/awt/PrintJob/PrintCheckboxTest/PrintCheckboxManualTest.java` Tests >> that there is no ClassCastException thrown in printing checkbox and >> scrollbar with XAWT. Error. Can't find HTML file: >> test\jdk\java\awt\PrintJob\PrintCheckboxTest\PrintCheckboxManualTest.html >> >> >> - `java/awt/print/PrinterJob/SecurityDialogTest.java` A windows with >> instructions is shown but it does not contain print/pass/fail buttons and >> it is not possible to close the window. >> >> - The tests below fail with "Error. Parse Exception: Arguments to `manual' >> option not supported: yesno" message: >> java/awt/print/Dialog/DialogOrient.java >> java/awt/print/Dialog/DialogType.java >> java/awt/print/PrinterJob/ImagePrinting/ClippedImages.java >> java/awt/print/PrinterJob/ImagePrinting/ImageTypes.java >> java/awt/print/PrinterJob/ImagePrinting/PrintARGBImage.java >> java/awt/print/PrinterJob/PageDialogTest.java >> java/awt/print/PrinterJob/PageRanges.java >> java/awt/print/PrinterJob/PageRangesDlgTest.java >> java/awt/print/PrinterJob/PrintGlyphVectorTest.java >> java/awt/print/PrinterJob/PrintLatinCJKTest.java >> java/awt/print/PrinterJob/PrintTextTest.java >> java/awt/print/PrinterJob/SwingUIText.java >> java/awt/PrintJob/ConstrainedPrintingTest/ConstrainedPrintingTest.java >> java/awt/PrintJob/PageSetupDlgBlockingTest/PageSetupDlgBlockingTest.java >> java/awt/PrintJob/SaveDialogTitleTest.java > > Marked as reviewed by serb (Reviewer). @prsadhuk please take a look - PR: https://git.openjdk.java.net/jdk/pull/2756
Re: [OpenJDK 2D-Dev] RFR: 8262470: Printed GlyphVector outline with low DPI has bad quality on Windows
On Fri, 26 Feb 2021 19:40:22 GMT, Alexander Scherbatiy wrote: > Printing text using GlyphVector outline has bad quality on printers with low > DPI on Windows. > The GDI system used for text printing on Windows accepts only integer path > coordinates. > Rounding GlyphVector outline coordinates leads to distorted printed text. > > The issue had been reported as JDK-8256264 but was reverted because of the > regression JDK-8259007 "This test printed a blank page". > > The fix JDK-8256264 scaled coordinates in wPrinterJob.moveTo()/lineTo() > methods up and scaled transforms in wPrinterJob.beginPath()/endPath() down. > > The regression was in the WPathGraphics.deviceDrawLine() method which uses > wPrinterJob.moveTo()/lineTo() methods without surrounding them with > wPrinterJob.beginPath()/endPath() so the line coordinates were only scaled up. > > I tried to put wPrinterJob.beginPath()/endPath() methods around > wPrinterJob.moveTo()/lineTo() in the method WPathGraphics.deviceDrawLine() > but the line was not drawn at all even without scaling coordinates up and > transform down (without JDK-8256264 fix). It looks like GDI treats this case > as an empty shape. > > The proposed fix applies path coordinates and transform scaling only in > WPathGraphics.convertToWPath() method. > The one more PathPrecisionScaleFactorShapeTest.java manual test is added > which checks that all methods that draw paths in WPathGraphics are used: line > in WPathGraphics.deviceDrawLine() and SEG_LINETO/SEG_QUADTO/SEG_CUBICTO in > WPathGraphics.convertToWPath() . > > The `java/awt/print` and `java/awt/PrintJob` automatic and manual tests were > run on Windows 10 Pro with the fix. > > There are two failed automated tests which fail without the fix as well: > java/awt/print/PrinterJob/GlyphPositions.java > java/awt/print/PrinterJob/PSQuestionMark.java > > The following manual tests have issues on my system: > - `java/awt/print/Dialog/PrintDlgPageable.java` > java.lang.IllegalAccessException: class > com.sun.javatest.regtest.agent.MainWrapper$MainThread cannot access a member > of class PrintDlgPageable with modifiers "public static" > > - `java/awt/print/PrinterJob/PrintAttributeUpdateTest.java` I select pages > radio button, press the print button but the test does not finish and I do > not see any other dialogs with pass/fail buttons. > > - `java/awt/PrintJob/PrintCheckboxTest/PrintCheckboxManualTest.java` Tests > that there is no ClassCastException thrown in printing checkbox and scrollbar > with XAWT. Error. Can't find HTML file: > test\jdk\java\awt\PrintJob\PrintCheckboxTest\PrintCheckboxManualTest.html > > > - `java/awt/print/PrinterJob/SecurityDialogTest.java` A windows with > instructions is shown but it does not contain print/pass/fail buttons and it > is not possible to close the window. > > - The tests below fail with "Error. Parse Exception: Arguments to `manual' > option not supported: yesno" message: > java/awt/print/Dialog/DialogOrient.java > java/awt/print/Dialog/DialogType.java > java/awt/print/PrinterJob/ImagePrinting/ClippedImages.java > java/awt/print/PrinterJob/ImagePrinting/ImageTypes.java > java/awt/print/PrinterJob/ImagePrinting/PrintARGBImage.java > java/awt/print/PrinterJob/PageDialogTest.java > java/awt/print/PrinterJob/PageRanges.java > java/awt/print/PrinterJob/PageRangesDlgTest.java > java/awt/print/PrinterJob/PrintGlyphVectorTest.java > java/awt/print/PrinterJob/PrintLatinCJKTest.java > java/awt/print/PrinterJob/PrintTextTest.java > java/awt/print/PrinterJob/SwingUIText.java > java/awt/PrintJob/ConstrainedPrintingTest/ConstrainedPrintingTest.java > java/awt/PrintJob/PageSetupDlgBlockingTest/PageSetupDlgBlockingTest.java > java/awt/PrintJob/SaveDialogTitleTest.java Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2756
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons
On Mon, 8 Mar 2021 13:23:00 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Continuation of review at https://github.com/openjdk/jdk/pull/380 Just to continue the thread from the old review, did you have a chance to look at the possibility of loading all existed icons embedded in the file? It was discussed here: https://github.com/openjdk/jdk/pull/380#issuecomment-702999573 - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Tue, 9 Mar 2021 22:11:20 GMT, Alexey Ushakov wrote: >> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceDataBase.h >> line 109: >> >>> 107: #define MTLSD_TEXTURE sun_java2d_pipe_hw_AccelSurface_TEXTURE >>> 108: #define MTLSD_FLIP_BACKBUFFER >>> sun_java2d_pipe_hw_AccelSurface_FLIP_BACKBUFFER >>> 109: #define MTLSD_RT_TEXTURE >>> sun_java2d_pipe_hw_AccelSurface_RT_TEXTURE >> >> Only two of them are supported? MTLSD_TEXTURE and MTLSD_RT_TEXTURE? > > Also, MTLSD_WINDOW is supported How it is used? Can we really draw to the window directly? It looks like it copied from the OGL where it is unused since CLayer integration. Something is inconsistent here. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8263138: Initialization of sun.font.SunFontManager.platformFontMap is not thread safe
On Tue, 9 Mar 2021 20:56:15 GMT, Alexey Ivanov wrote: >> 8263138: Initialization of sun.font.SunFontManager.platformFontMap is not >> thread safe > > Marked as reviewed by aivanov (Reviewer). > I don't know why making this volatile is doing anything. > Let's discuss this further. It will prevent the possibility that one thread will store the non-null value to the SunFontManager.platformFontMap and only after that complete initialization of object stored in that field due to code optimizations. It is similar to the volatile DCL: see "broken multithreaded version" example: https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java - PR: https://git.openjdk.java.net/jdk/pull/2762
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Tue, 9 Mar 2021 20:04:17 GMT, Alexey Ushakov wrote: >> Probably it's enough to set the context in SET_SURFACES, I'll double-check >> it. > > Looks like it works even without SET_SCRATCH_SURFACE (I did some limited > testing) but I don't think we should do such a huge change at this moment. JDK-8263309 - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Tue, 9 Mar 2021 20:13:03 GMT, Sergey Bylokhov wrote: >> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line >> 676: >> >>> 674: height = srcInfo.bounds.y2 - srcInfo.bounds.y1; >>> 675: >>> 676: pDst = PtrAddBytes(pDst, dstx * dstInfo.pixelStride); >> >> Here and in other places use the PtrPixelsRow instead, do not use >> multiplication, see the history of the OGLBlitLoops_SurfaceToSwBlit method. > > I think this at least should be investigated before integration. JDK-8263324 - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v11]
On Tue, 9 Mar 2021 19:52:35 GMT, Phil Race wrote: >> Ajit Ghaisas 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 41 additional >> commits since the last revision: >> >> - Lanai PR#210 - 8263159 - jdv >> - Lanai PR#209 - 8262936 - jdv >> - Lanai PR#208 - 8262928 - jdv >> - Lanai PR#207 - 8262750 - jdv >> - Merge branch 'master' into 8260931_lanai_JEP_branch >> - Lanai PR#206 - 8262729 - aghaisas >> - Lanai PR#205 - 8262496 - avu >> - Lanai PR#203 - 8262313 - jdv >> - Lanai PR#202 - 8262293 - avu >> - Lanai PR#201 - 8261891 - avu >> - ... and 31 more: >> https://git.openjdk.java.net/jdk/compare/1bd8aea0...de456939 > > test/jdk/performance/client/RenderPerfTest/build.xml line 72: > >> 70: >> 71: >> 72: > > This was presumably borrowed from J2DBench so this comment should be fixed ! JDK-8263325 - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Sun, 7 Mar 2021 22:31:16 GMT, Sergey Bylokhov wrote: >> Ajit Ghaisas 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 36 additional >> commits since the last revision: >> >> - Lanai PR#206 - 8262729 - aghaisas >> - Lanai PR#205 - 8262496 - avu >> - Lanai PR#203 - 8262313 - jdv >> - Lanai PR#202 - 8262293 - avu >> - Lanai PR#201 - 8261891 - avu >> - Lanai PR#200 - 8262115 - aghaisas >> - Merge branch 'master' into 8260931_lanai_JEP_branch >> - Lanai PR#199 - 8262091 - aghaisas >> - Lanai PR#198 - 8261646 - avu >> - Lanai PR#197 - 8261960 - jdv >> - ... and 26 more: >> https://git.openjdk.java.net/jdk/compare/2f09d382...5cb1fd91 > > src/java.desktop/macosx/classes/sun/java2d/metal/MTLLayer.java line 44: > >> 42: >> 43: // Pass the insets to native code to make adjustments in blitTexture >> 44: private static native void nativeSetInsets(long layerPtr, int top, >> int left); > > Probably I have asked this already, but why we need to use insets? Why insets > is not necessary for ogl? We have different coordinate systems for OGL and Metal. Metal (0,0) for textures is the top-left corner whereas OGL (0,0) is the bottom-left one, so we need to have insets to perform blitting to drawable correctly. > src/java.desktop/macosx/classes/sun/java2d/metal/MTLBlitLoops.java line 499: > >> 497: } >> 498: >> 499: // We can convert argb_pre data from MTL surface in two places: > > Does anybody check that this is true for the metal pipeline? or It is just > copied from the OGL? I fixed some conversion logic within JDK-8256331. > src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java line > 168: > >> 166: } >> 167: >> 168: ContextCapabilities caps = new MTLContext.MTLContextCaps( > > CAPS_DOUBLEBUFFERED is not included? JDK-8263306 > src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 146: > >> 144: return MTLSurfaceRTT; >> 145: default: >> 146: return MTLSurface; > > Do we really support 3 different types of surface? I guess only two of them > are different: textures used for manageable images, and surfaces used by the > volatile image. It's true but I don't see any problem returning a more generic type as a default here > src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 221: > >> 219: protected native boolean initRTexture(long pData, boolean isOpaque, >> int width, int height); >> 220: >> 221: protected native boolean initFlipBackbuffer(long pData); > > flip back buffer is supported? No, it should be removed (JDK-8263312) > src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceData.m > line 284: > >> 282: */ >> 283: jboolean >> 284: MTLSD_InitMTLWindow(JNIEnv *env, BMTLSDOps *bmtlsdo) > > When the MTLWindow is used? MTLContext setSurfacesEnv > src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceDataBase.h > line 109: > >> 107: #define MTLSD_TEXTURE sun_java2d_pipe_hw_AccelSurface_TEXTURE >> 108: #define MTLSD_FLIP_BACKBUFFER >> sun_java2d_pipe_hw_AccelSurface_FLIP_BACKBUFFER >> 109: #define MTLSD_RT_TEXTURE >> sun_java2d_pipe_hw_AccelSurface_RT_TEXTURE > > Only two of them are supported? MTLSD_TEXTURE and MTLSD_RT_TEXTURE? Also, MTLSD_WINDOW is supported - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Tue, 9 Mar 2021 17:59:26 GMT, Alexey Ushakov wrote: >> In fact, we don't have any scratch surfaces. SET_SCRATCH_SURFACE effectively >> sets the new context. For better readability, we should add the new op >> SET_CONTEXT in BufferedOpCodes > > Probably it's enough to set the context in SET_SURFACES, I'll double-check it. Looks like it works even without SET_SCRATCH_SURFACE (I did some limited testing) but I don't think we should do such a huge change at this moment. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Tue, 9 Mar 2021 17:26:24 GMT, Alexey Ushakov wrote:
>> src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 68:
>>
>>> 66: * when disposing a texture-based surface).
>>> 67: */
>>> 68: public static void setScratchSurface(long pConfigInfo) {
>>
>> How the scratch surface is used in the metal pipeline? Why it is not enough
>> to set the "context current"?
>
> In fact, we don't have any scratch surfaces. SET_SCRATCH_SURFACE effectively
> sets the new context. For better readability, we should add the new op
> SET_CONTEXT in BufferedOpCodes
Probably it's enough to set the context in SET_SURFACES, I'll double-check it.
>> src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 140:
>>
>>> 138: StringBuilder sb = new StringBuilder(super.toString());
>>> 139: if ((caps & CAPS_DOUBLEBUFFERED) != 0) {
>>> 140: sb.append("CAPS_DOUBLEBUFFERED|");
>>
>> Related to other questions, we do not include CAPS_DOUBLEBUFFERED, but
>> anyway, report the surface as double buffered.
>
> We can remove all the unused constants
JDK-8263306
-
PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Mon, 1 Mar 2021 11:17:39 GMT, Ajit Ghaisas wrote: >> **Description :** >> This is the implementation of [JEP 382 : New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361) >> It implements a Java 2D internal rendering pipeline for macOS using the >> Apple Metal API. >> The entire work on this was done under [OpenJDK Project - >> Lanai](http://openjdk.java.net/projects/lanai/) >> >> We iterated through several Early Access (EA) builds and have reached a >> stage where it is ready to be integrated to openjdk/jdk. The latest EA build >> is available at - https://jdk.java.net/lanai/ >> >> A new option -Dsun.java2d.metal=true | True needs to be used to use this >> pipeline. >> >> **Testing :** >> This implementation has been tested with the tests present at - [Test Plan >> for JEP 382: New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396) >> >> **Note to reviewers :** >> 1) Default rendering pipeline on macOS has not been changed by this PR. >> OpenGL still stays as the default rendering pipeline and Metal rendering >> pipeline is optional to choose. >> >> 2) To apply and test this PR - >> To enable the metal pipeline you must specify on command line >> -Dsun.java2d.metal=true (No message will be printed in this case) or >> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is >> enabled gets printed) >> >> 3) Review comments (including some preliminary informal review comments) are >> tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598 > > Ajit Ghaisas 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 36 additional > commits since the last revision: > > - Lanai PR#206 - 8262729 - aghaisas > - Lanai PR#205 - 8262496 - avu > - Lanai PR#203 - 8262313 - jdv > - Lanai PR#202 - 8262293 - avu > - Lanai PR#201 - 8261891 - avu > - Lanai PR#200 - 8262115 - aghaisas > - Merge branch 'master' into 8260931_lanai_JEP_branch > - Lanai PR#199 - 8262091 - aghaisas > - Lanai PR#198 - 8261646 - avu > - Lanai PR#197 - 8261960 - jdv > - ... and 26 more: > https://git.openjdk.java.net/jdk/compare/8f9013c3...5cb1fd91 Looks good, but a couple of things should be fixed (JDK-8263325, JDK-8263324) - Changes requested by avu (no project role). PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8263138: Initialization of sun.font.SunFontManager.platformFontMap is not thread safe
On Sat, 27 Feb 2021 18:33:52 GMT, Andrey Turbanov wrote: > 8263138: Initialization of sun.font.SunFontManager.platformFontMap is not > thread safe Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2762
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Sun, 7 Mar 2021 22:48:47 GMT, Sergey Bylokhov wrote: >> Ajit Ghaisas 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 36 additional >> commits since the last revision: >> >> - Lanai PR#206 - 8262729 - aghaisas >> - Lanai PR#205 - 8262496 - avu >> - Lanai PR#203 - 8262313 - jdv >> - Lanai PR#202 - 8262293 - avu >> - Lanai PR#201 - 8261891 - avu >> - Lanai PR#200 - 8262115 - aghaisas >> - Merge branch 'master' into 8260931_lanai_JEP_branch >> - Lanai PR#199 - 8262091 - aghaisas >> - Lanai PR#198 - 8261646 - avu >> - Lanai PR#197 - 8261960 - jdv >> - ... and 26 more: >> https://git.openjdk.java.net/jdk/compare/af4701dd...5cb1fd91 > > src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line > 676: > >> 674: height = srcInfo.bounds.y2 - srcInfo.bounds.y1; >> 675: >> 676: pDst = PtrAddBytes(pDst, dstx * dstInfo.pixelStride); > > Here and in other places use the PtrPixelsRow instead, do not use > multiplication, see the history of the OGLBlitLoops_SurfaceToSwBlit method. I think this at least should be investigated before integration. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v11]
On Mon, 8 Mar 2021 08:06:03 GMT, Ajit Ghaisas wrote: >> **Description :** >> This is the implementation of [JEP 382 : New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361) >> It implements a Java 2D internal rendering pipeline for macOS using the >> Apple Metal API. >> The entire work on this was done under [OpenJDK Project - >> Lanai](http://openjdk.java.net/projects/lanai/) >> >> We iterated through several Early Access (EA) builds and have reached a >> stage where it is ready to be integrated to openjdk/jdk. The latest EA build >> is available at - https://jdk.java.net/lanai/ >> >> A new option -Dsun.java2d.metal=true | True needs to be used to use this >> pipeline. >> >> **Testing :** >> This implementation has been tested with the tests present at - [Test Plan >> for JEP 382: New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396) >> >> **Note to reviewers :** >> 1) Default rendering pipeline on macOS has not been changed by this PR. >> OpenGL still stays as the default rendering pipeline and Metal rendering >> pipeline is optional to choose. >> >> 2) To apply and test this PR - >> To enable the metal pipeline you must specify on command line >> -Dsun.java2d.metal=true (No message will be printed in this case) or >> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is >> enabled gets printed) >> >> 3) Review comments (including some preliminary informal review comments) are >> tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598 > > Ajit Ghaisas 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 41 additional > commits since the last revision: > > - Lanai PR#210 - 8263159 - jdv > - Lanai PR#209 - 8262936 - jdv > - Lanai PR#208 - 8262928 - jdv > - Lanai PR#207 - 8262750 - jdv > - Merge branch 'master' into 8260931_lanai_JEP_branch > - Lanai PR#206 - 8262729 - aghaisas > - Lanai PR#205 - 8262496 - avu > - Lanai PR#203 - 8262313 - jdv > - Lanai PR#202 - 8262293 - avu > - Lanai PR#201 - 8261891 - avu > - ... and 31 more: > https://git.openjdk.java.net/jdk/compare/851d3def...de456939 src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m line 185: > 183: > 184: NSRect contentRect = NSMakeRect(0, 0, 64, 64); > 185: NSWindow *window = Some remnant from the scratch surface initialization? - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v8]
On Mon, 15 Feb 2021 20:55:13 GMT, Phil Race wrote: >> Marked as reviewed by gziemski (Committer). > >> > > Thanks @gerard-ziemski for taking a detailed look at this. >> > > We do have an open bug to address this. Please refer >> > > [JDK-8259825](https://bugs.openjdk.java.net/browse/JDK-8259825). >> > >> > >> > Hi Gerard, expecting a process and parsing its output is definitely not >> > ideal and that's why there's this open bug. >> > One thing that is under consideration is to equate >= 10.14 with Metal is >> > available since as of 10.14 macOS won't install if a system does not >> > support metal. We have no compelling reason to support metal on earlier >> > releases anyway .. those are either already unsupported or barely >> > supported and OGL will always be available there. >> >> I did not know that there already was a bug covering this issue. >> >> Your idea seems reasonable. >> >> More than just focusing on this immediate issue, however, I was hoping to >> raise the point of us starting adopting profiling tools to analyze our code >> (memory utilization, leaks, cpu/gpu profiling etc). A new feature that >> brings 10% benefit, but uses 50% more resources for example would probably >> not be a good investment. And to figure that we need to relay on some good >> tools and Xcode provides some. > > There were actually tasks to do profiling of the memory footprint and look > for leaks. We did not think it possible to be able to assert "Metal must use > less memory than OpenGL" or dig into tiny differences but it was intended to > find the big issues. See https://bugs.openjdk.java.net/browse/JDK-8237856 > @prsadhuk maybe able to say how much of it was doing using Xcode. Regarding this comment from @mrserb > Probably place it near J2Dbench? I can't find the context but if it is suggesting moving RenderPerfTest to be co-located with J2Bench let's NOT do that. It is much more likely that J2DBench will be moved out of demo into test ... - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v11]
On Mon, 8 Mar 2021 08:06:03 GMT, Ajit Ghaisas wrote: >> **Description :** >> This is the implementation of [JEP 382 : New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361) >> It implements a Java 2D internal rendering pipeline for macOS using the >> Apple Metal API. >> The entire work on this was done under [OpenJDK Project - >> Lanai](http://openjdk.java.net/projects/lanai/) >> >> We iterated through several Early Access (EA) builds and have reached a >> stage where it is ready to be integrated to openjdk/jdk. The latest EA build >> is available at - https://jdk.java.net/lanai/ >> >> A new option -Dsun.java2d.metal=true | True needs to be used to use this >> pipeline. >> >> **Testing :** >> This implementation has been tested with the tests present at - [Test Plan >> for JEP 382: New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396) >> >> **Note to reviewers :** >> 1) Default rendering pipeline on macOS has not been changed by this PR. >> OpenGL still stays as the default rendering pipeline and Metal rendering >> pipeline is optional to choose. >> >> 2) To apply and test this PR - >> To enable the metal pipeline you must specify on command line >> -Dsun.java2d.metal=true (No message will be printed in this case) or >> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is >> enabled gets printed) >> >> 3) Review comments (including some preliminary informal review comments) are >> tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598 > > Ajit Ghaisas 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 41 additional > commits since the last revision: > > - Lanai PR#210 - 8263159 - jdv > - Lanai PR#209 - 8262936 - jdv > - Lanai PR#208 - 8262928 - jdv > - Lanai PR#207 - 8262750 - jdv > - Merge branch 'master' into 8260931_lanai_JEP_branch > - Lanai PR#206 - 8262729 - aghaisas > - Lanai PR#205 - 8262496 - avu > - Lanai PR#203 - 8262313 - jdv > - Lanai PR#202 - 8262293 - avu > - Lanai PR#201 - 8261891 - avu > - ... and 31 more: > https://git.openjdk.java.net/jdk/compare/32b236e4...de456939 test/jdk/performance/client/RenderPerfTest/build.xml line 72: > 70: > 71: > 72: This was presumably borrowed from J2DBench so this comment should be fixed ! - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v21]
On Mon, 1 Mar 2021 10:31:19 GMT, Andrew Haley wrote: >> Anton Kozlov has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos >> - Minor fixes > > src/hotspot/cpu/aarch64/globalDefinitions_aarch64.hpp line 62: > >> 60: >> 61: #if defined(__APPLE__) || defined(_WIN64) >> 62: #define R18_RESERVED > > #define R18_RESERVED true``` We always check for `R18_RESERVED` with `#if(n)def`, is there any reason to define the value for the macro? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v12]
On Tue, 9 Feb 2021 09:23:50 GMT, Stefan Karlsson wrote:
>> Anton Kozlov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update signal handler part for debugger
>
> src/hotspot/share/runtime/thread.cpp line 2515:
>
>> 2513: void JavaThread::check_special_condition_for_native_trans(JavaThread
>> *thread) {
>> 2514: // Enable WXWrite: called directly from interpreter native wrapper.
>> 2515: MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXWrite, thread));
>
> FWIW, I personally think that adding these MACOS_AARCH64_ONLY usages at the
> call sites increase the line-noise in the affected functions. I think I would
> have preferred a version:
> ThreadWXEnable(WXMode new_mode, Thread* thread = NULL) {
> MACOS_AARCH64_ONLY(initialize(new_mode, thread);) {}
> void initialize(...); // Implementation in thread_bsd_aarch64.cpp (alt.
> inline.hpp)
> With that said, I'm fine with taking this discussion as a follow-up.
The former version used no such macros. I like that now it's clear the W^X
management is relevant to macos/aarch64 only. I see the point to move the
pre-processor condition into the class implementation. But I think it will
bring a bit of inconsistency, as the rest of W^X implementation is explicitly
guarded by preprocessor conditionals. I've also tried to push macro
conditionals as far as possible down to implementation, providing a kind of
generalized W^X interface. That required a few artificial decisions, e.g. how
would we call the mode we execute on the rest of platforms with write and
execute allowed, WXWriteExec?.. I abandoned that attempt.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Sun, 7 Mar 2021 22:29:50 GMT, Sergey Bylokhov wrote:
>> Ajit Ghaisas 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 36 additional
>> commits since the last revision:
>>
>> - Lanai PR#206 - 8262729 - aghaisas
>> - Lanai PR#205 - 8262496 - avu
>> - Lanai PR#203 - 8262313 - jdv
>> - Lanai PR#202 - 8262293 - avu
>> - Lanai PR#201 - 8261891 - avu
>> - Lanai PR#200 - 8262115 - aghaisas
>> - Merge branch 'master' into 8260931_lanai_JEP_branch
>> - Lanai PR#199 - 8262091 - aghaisas
>> - Lanai PR#198 - 8261646 - avu
>> - Lanai PR#197 - 8261960 - jdv
>> - ... and 26 more:
>> https://git.openjdk.java.net/jdk/compare/69ed64f1...5cb1fd91
>
> src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 140:
>
>> 138: StringBuilder sb = new StringBuilder(super.toString());
>> 139: if ((caps & CAPS_DOUBLEBUFFERED) != 0) {
>> 140: sb.append("CAPS_DOUBLEBUFFERED|");
>
> Related to other questions, we do not include CAPS_DOUBLEBUFFERED, but
> anyway, report the surface as double buffered.
We can remove all the unused constants
-
PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v10]
On Sun, 7 Mar 2021 22:24:54 GMT, Sergey Bylokhov wrote:
>> Ajit Ghaisas 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 36 additional
>> commits since the last revision:
>>
>> - Lanai PR#206 - 8262729 - aghaisas
>> - Lanai PR#205 - 8262496 - avu
>> - Lanai PR#203 - 8262313 - jdv
>> - Lanai PR#202 - 8262293 - avu
>> - Lanai PR#201 - 8261891 - avu
>> - Lanai PR#200 - 8262115 - aghaisas
>> - Merge branch 'master' into 8260931_lanai_JEP_branch
>> - Lanai PR#199 - 8262091 - aghaisas
>> - Lanai PR#198 - 8261646 - avu
>> - Lanai PR#197 - 8261960 - jdv
>> - ... and 26 more:
>> https://git.openjdk.java.net/jdk/compare/ffb3d48c...5cb1fd91
>
> src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 68:
>
>> 66: * when disposing a texture-based surface).
>> 67: */
>> 68: public static void setScratchSurface(long pConfigInfo) {
>
> How the scratch surface is used in the metal pipeline? Why it is not enough
> to set the "context current"?
In fact, we don't have any scratch surfaces. SET_SCRATCH_SURFACE effectively
sets the new context. For better readability, we should add the new op
SET_CONTEXT in BufferedOpCodes
-
PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v12]
On Tue, 9 Feb 2021 09:12:13 GMT, Stefan Karlsson wrote: >> Anton Kozlov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update signal handler part for debugger > > src/hotspot/share/runtime/thread.hpp line 848: > >> 846: void init_wx(); >> 847: WXMode enable_wx(WXMode new_state); >> 848: #endif // __APPLE__ && AARCH64 > > Now that this is only compiled into macOS/AArch64, could this be moved over > to thread_bsd_aarch64.hpp? The same goes for the associated functions. The thread_bsd_aarch64.hpp describes a part of JavaThread, while this block belongs to Thread for now. Since W^X is an attribute of any operating system thread, I assumed Thread to be the right place for W^X bookkeeping. In most cases, we manage W^X state of JavaThread. But sometimes a GC thread needs the WXWrite state, or safefetch is called from non-JavaThread. Probably this can be dealt with (e.g. GCThread to always have the WXWrite state). But such change would be much more than a simple refactoring and it would require a significant amount of testing. Ideally, I would like to investigate this as a follow-up change, or at least after other fixes to this PR. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v24]
> Please review the implementation of JEP 391: macOS/AArch64 Port. > > It's heavily based on existing ports to linux/aarch64, macos/x86_64, and > windows/aarch64. > > Major changes are in: > * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks > JDK-8253817, JDK-8253818) > * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with necessary > adjustments (JDK-8253819) > * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), > required on macOS/AArch64 platform. It's implemented with > pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a > thread, so W^X mode change relates to the java thread state change (for java > threads). In most cases, JVM executes in write-only mode, except when calling > a generated stub like SafeFetch, which requires a temporary switch to > execute-only mode. The same execute-only mode is enabled when a java thread > executes in java or native states. This approach of managing W^X mode turned > out to be simple and efficient enough. > * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) Anton Kozlov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 105 commits: - Merge commit 'refs/pull/11/head' of https://github.com/AntonKozlov/jdk into jdk-macos - workaround JDK-8262895 by disabling subtest - Fix typo - Rename threadWXSetters.hpp -> threadWXSetters.inline.hpp - JDK-8259937: bsd_aarch64 part - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos - Fix after JDK-8259539, partially revert preconditions - JDK-8260471: bsd_aarch64 part - JDK-8259539: bsd_aarch64 part - JDK-8257828: bsd_aarch64 part - ... and 95 more: https://git.openjdk.java.net/jdk/compare/a6e34b3d...a72f6834 - Changes: https://git.openjdk.java.net/jdk/pull/2200/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2200=23 Stats: 2873 lines in 73 files changed: 2787 ins; 27 del; 59 mod Patch: https://git.openjdk.java.net/jdk/pull/2200.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2200/head:pull/2200 PR: https://git.openjdk.java.net/jdk/pull/2200
Re: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v12]
On Tue, 9 Feb 2021 09:06:26 GMT, Stefan Karlsson wrote: >> Anton Kozlov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update signal handler part for debugger > > src/hotspot/share/runtime/threadWXSetters.hpp line 28: > >> 26: #define SHARE_RUNTIME_THREADWXSETTERS_HPP >> 27: >> 28: #include "runtime/thread.inline.hpp" > > This breaks against our convention to forbid inline.hpp files from being > included from being included from .hpp files. You need to rework this by > either moving the implementation to a .cpp file, or convert this file into an > .inline.hpp > > See the Source Files section in: > https://htmlpreview.github.io/?https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.html Thanks, I renamed the file to threadWXSetters.inline.hpp - PR: https://git.openjdk.java.net/jdk/pull/2200
[OpenJDK 2D-Dev] Integrated: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()
On Thu, 4 Mar 2021 21:37:33 GMT, Alexey Ivanov wrote: > **Root cause:** > `getPrinterNames()` has two calls to > [`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters). > The first call is to get the required buffer size to contain the structures > and any strings. The second call is to get the list of printers. > > Yet the list of printers or the names of printers can change between the two > calls. If it happens, the second call to `EnumPrinters` fails but it is not > checked. > > I couldn't reproduce the crash myself. However, I reproduced the failure in > the second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming > the printers so that the data doesn't fit into the allocated buffer: > > 1: bResult: 0, cbNeeded: 504, cReturned: 0 > 2: bResult: 0, cbNeeded: 512, cReturned: 0 > !!! error > > During my testing, `cReturned` has always been zero whenever `EnumPrinters` > fails. > > The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` > buffer doesn't contain valid data. Reading `info4->pPrinterName` at the line > utf_str = JNU_NewStringPlatform(env, info4->pPrinterName); > raises Access Violation exception. > > **The fix:** > Check the return value of `EnumPrinters` and allow for 5 retries. If the > function still returns failure, make sure `cReturned` is set to zero. > > I'm going to close > [JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and > [JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported > previously about the same crash as duplicate of the current > [JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829). > > **Testing:** > I used > [`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java) > for testing, it shows the list of currently available printers. In the > background I ran `PrinterRename.ps1` PowerShell script which remains a > printer, the script is attached to the JBS issue. > > Without the fix, the list of available printers was empty occasionally > because `EnumPrinters` returned failure and set `cReturned` to zero. With the > fix, the list of printers is always filled. This pull request has now been integrated. Changeset: a6e34b3d Author:Alexey Ivanov URL: https://git.openjdk.java.net/jdk/commit/a6e34b3d Stats: 20 lines in 1 file changed: 13 ins; 0 del; 7 mod 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames() Reviewed-by: prr, psadhukhan, serb - PR: https://git.openjdk.java.net/jdk/pull/2836
