Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]
On Thu, 11 Feb 2021 05:41:15 GMT, Ajit Ghaisas wrote: >> According to Xcode Instruments leak profile, there are 2 minor memory leaks >> in the Metal rendering pipeline: >> >> `#1 Malloc 80 Bytes 1 0x7fde0d4247b0 80 Byteslibjava.dylib >> getStringUTF8` >>0 libsystem_malloc.dylib malloc_zone_malloc >>1 libsystem_malloc.dylib malloc >>2 libjava.dylib getStringUTF8 >> /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888 >>3 libjava.dylib JNU_GetStringPlatformChars >> /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917 >>4 libawt_lwawt.dylib >> Java_sun_java2d_metal_MTLGraphicsConfig_getMTLConfigInfo >> /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:151 >>5 0x11ab08d48 >>6 0x11ab0250d >> >> `#2 Malloc 80 Bytes 1 0x7fde0d4325a0 80 Byteslibjava.dylib >> getStringUTF8` >>0 libsystem_malloc.dylib malloc_zone_malloc >>1 libsystem_malloc.dylib malloc >>2 libjava.dylib getStringUTF8 >> /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888 >>3 libjava.dylib JNU_GetStringPlatformChars >> /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917 >>4 libawt_lwawt.dylib >> Java_sun_java2d_metal_MTLGraphicsConfig_tryLoadMetalLibrary >> /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:120 >>5 0x11ab08d48 >>6 0x11ab024c8 >> >> Those can be handled as a followup issues though if you like, it's only 160 >> bytes total. > >> I tried to code review the native implementation files, but Metal APIs is >> brand new to me and it's been a long while since I worked with graphics API, >> so I can't be of much help really. >> >> The code I've looked at looked clean and nothing caught my eye. But it's a >> partial code review at best. >> >> Good job! > > Every bit helps. Thanks for your review effort! > A small thing caught my eye when I was comparing OpenGL and Metal pipelines > running J2Demo.jar > > It looks to me like OpenGL rendering pipeline uses brighter colors. I prefer > the more subdued colors that Metal is rendering, but not sure if this is > something you want to investigate further. > > I tried to capture what I see below: > > OpenGL: > > src="https://user-images.githubusercontent.com/63425797/107575744-dc0ca180-6bb5-11eb-9eb3-5cff415eb8a3.png";> > > Metal (the green color has a yellowish tint): > > src="https://user-images.githubusercontent.com/63425797/107575716-cf884900-6bb5-11eb-8ae8-14212ec94e87.png";> > > Performance wise I did not see much difference either way. @gerard-ziemski Thanks for verifying the behavior. Text present in Memory monitor/Performance in J2DDemo is not drawn using Metal rendering pipeline(drawGlyphList or any other rendering opcodes, I have verified it again in latest code). This is drawn using software renderloops and only thing happens from Metal pipeline is this content is used in blit opcode. Slight difference in color that we are seeing is related to how Metal internally handles blending of colors. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]
On Wed, 10 Feb 2021 22:31:12 GMT, Gerard Ziemski wrote: >> Marked as reviewed by gziemski (Committer). > > According to Xcode Instruments leak profile, there are 2 minor memory leaks > in the Metal rendering pipeline: > > `#1 Malloc 80 Bytes 1 0x7fde0d4247b0 80 Byteslibjava.dylib > getStringUTF8` >0 libsystem_malloc.dylib malloc_zone_malloc >1 libsystem_malloc.dylib malloc >2 libjava.dylib getStringUTF8 > /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888 >3 libjava.dylib JNU_GetStringPlatformChars > /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917 >4 libawt_lwawt.dylib > Java_sun_java2d_metal_MTLGraphicsConfig_getMTLConfigInfo > /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:151 >5 0x11ab08d48 >6 0x11ab0250d > > `#2 Malloc 80 Bytes 1 0x7fde0d4325a0 80 Byteslibjava.dylib > getStringUTF8` >0 libsystem_malloc.dylib malloc_zone_malloc >1 libsystem_malloc.dylib malloc >2 libjava.dylib getStringUTF8 > /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888 >3 libjava.dylib JNU_GetStringPlatformChars > /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917 >4 libawt_lwawt.dylib > Java_sun_java2d_metal_MTLGraphicsConfig_tryLoadMetalLibrary > /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:120 >5 0x11ab08d48 >6 0x11ab024c8 > > Those can be handled as a followup issues though if you like, it's only 160 > bytes total. > I tried to code review the native implementation files, but Metal APIs is > brand new to me and it's been a long while since I worked with graphics API, > so I can't be of much help really. > > The code I've looked at looked clean and nothing caught my eye. But it's a > partial code review at best. > > Good job! Every bit helps. Thanks for your review effort! - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]
On Wed, 10 Feb 2021 21:45:37 GMT, Gerard Ziemski wrote: >> Ajit Ghaisas has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Lanai PR#177 - 8261430 - aghaisas >> - Lanai PR#176 - 8261399 - jdv > > Marked as reviewed by gziemski (Committer). According to Xcode Instruments leak profile, there are 2 minor memory leaks in the Metal rendering pipeline: `#1 Malloc 80 Bytes 1 0x7fde0d4247b0 80 Byteslibjava.dylib getStringUTF8` 0 libsystem_malloc.dylib malloc_zone_malloc 1 libsystem_malloc.dylib malloc 2 libjava.dylib getStringUTF8 /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888 3 libjava.dylib JNU_GetStringPlatformChars /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917 4 libawt_lwawt.dylib Java_sun_java2d_metal_MTLGraphicsConfig_getMTLConfigInfo /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:151 5 0x11ab08d48 6 0x11ab0250d `#2 Malloc 80 Bytes 1 0x7fde0d4325a0 80 Byteslibjava.dylib getStringUTF8` 0 libsystem_malloc.dylib malloc_zone_malloc 1 libsystem_malloc.dylib malloc 2 libjava.dylib getStringUTF8 /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888 3 libjava.dylib JNU_GetStringPlatformChars /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917 4 libawt_lwawt.dylib Java_sun_java2d_metal_MTLGraphicsConfig_tryLoadMetalLibrary /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:120 5 0x11ab08d48 6 0x11ab024c8 Those can be handled as a followup issues though if you like, it's only 160 bytes total. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]
On Tue, 9 Feb 2021 12:29:52 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 incrementally with two additional > commits since the last revision: > > - Lanai PR#177 - 8261430 - aghaisas > - Lanai PR#176 - 8261399 - jdv Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]
On Tue, 9 Feb 2021 12:29:52 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 incrementally with two additional > commits since the last revision: > > - Lanai PR#177 - 8261430 - aghaisas > - Lanai PR#176 - 8261399 - jdv src/java.desktop/macosx/classes/sun/java2d/metal/MTLBlitLoops.java line 110: > 108: // MTLSurfaceData.PF_BYTE_GRAY), > 109: // new MTLSwToSurfaceBlit(SurfaceType.UshortGray, > 110: // MTLSurfaceData.PF_USHORT_GRAY), Probably we could remove this commented lines - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]
> **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) Ajit Ghaisas has updated the pull request incrementally with two additional commits since the last revision: - Lanai PR#177 - 8261430 - aghaisas - Lanai PR#176 - 8261399 - jdv - Changes: - all: https://git.openjdk.java.net/jdk/pull/2403/files - new: https://git.openjdk.java.net/jdk/pull/2403/files/6044adc0..64173289 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=02-03 Stats: 49 lines in 14 files changed: 0 ins; 43 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2403.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403 PR: https://git.openjdk.java.net/jdk/pull/2403
