Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v8]
On Mon, 15 Feb 2021 06:24:13 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 20 additional > commits since the last revision: > > - Lanai PR#191 - 8261705 - jdv > - Lanai PR#190 - 8261706 - jdv > - Lanai PR#189 - 8261712 - avu > - Lanai PR#187 - 8261704 - jdv > - Lanai PR#186 - 8261638 - avu > - Lanai PR#185 - 8261632 - jdv > - Lanai PR#184 - 8261620 - aghaisas > - Lanai PR#182 - 8261547 - psadhukhan > - Merge branch 'master' into 8260931_lanai_JEP_branch > - Lanai PR#181 - 8261143 - aghaisas > - ... and 10 more: > https://git.openjdk.java.net/jdk/compare/827ab648...7b0b0dc4 Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Mon, 15 Feb 2021 18:30:51 GMT, Gerard Ziemski wrote: >> Changes requested by gziemski (Committer). > > I took a look at https://bugs.openjdk.java.net/browse/JDK-8261408 and noticed > a startup time regression with the Metal rendering pipeline, so I dug a bit > and here is what I found using Xcode startup profiler: > > Here is the OpenGL pipeline: > src="https://user-images.githubusercontent.com/63425797/107981859-8efb4780-6f88-11eb-849a-385c29f3ff6f.png;> > > > Here is the Metal pipeline: > src="https://user-images.githubusercontent.com/63425797/107981882-9ae70980-6f88-11eb-9b73-d1bd19dfa07f.png;> > > If I read the results correctly, there is a weird 330ms time gap in the case > of the Metal pipeline where nothing is done and it looks like the culprit is > `Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable` > > The OpenGL pipeline gets blocked only for 83ms in > `Java_sun_java2d_opengl_CGLGraphicsConfig_getCGLConfigInfo` > > I would advice that we take a closer look at why > 'Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable' takes so > long, and optimize it, or cache the results, so the next VM instance can skip > it if needed (assuming the call doesn't end up initializing the native Metal > or something like that), still worth taking a look. > > I would also recommend that you adopt Xcode and its Instruments profiler > tools for future work. Please let me know if you need help in this area. > > I only scratched the surface here and I think that there is plenty of > profiling that can be done to investigate the startup time regression further. > > 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. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Thu, 11 Feb 2021 20:55:47 GMT, Gerard Ziemski wrote: >> Ajit Ghaisas has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Lanai PR#181 - 8261143 - aghaisas >> - Lanai PR#180 - 8261546 - jdv > > Changes requested by gziemski (Committer). I took a look at https://bugs.openjdk.java.net/browse/JDK-8261408 and noticed a startup time regression with the Metal rendering pipeline, so I dug a bit and here is what I found using Xcode startup profiler: Here is the OpenGL pipeline: https://user-images.githubusercontent.com/63425797/107981859-8efb4780-6f88-11eb-849a-385c29f3ff6f.png;> Here is the Metal pipeline: https://user-images.githubusercontent.com/63425797/107981882-9ae70980-6f88-11eb-9b73-d1bd19dfa07f.png;> If I read the results correctly, there is a weird 330ms time gap in the case of the Metal pipeline where nothing is done and it looks like the culprit is `Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable` The OpenGL pipeline gets blocked only for 83ms in `Java_sun_java2d_opengl_CGLGraphicsConfig_getCGLConfigInfo` I would advice that we take a closer look at why 'Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable' takes so long, and optimize it, or cache the results, so the next VM instance can skip it if needed (assuming the call doesn't end up initializing the native Metal or something like that), still worth taking a look. I would also recommend that you adopt Xcode and its Instruments profiler tools for future work. Please let me know if you need help in this area. I only scratched the surface here and I think that there is plenty of profiling that can be done to investigate the startup time regression further. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Thu, 11 Feb 2021 20:58:36 GMT, Gerard Ziemski wrote: >> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 112: >> >>> 110: sourceSize:MTLSizeMake(self.buffer.width, >>> self.buffer.height, 1) >>> 111: toTexture:mtlDrawable.texture destinationSlice:0 >>> destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; >>> 112: [blitEncoder endEncoding]; >> >> There is an issue with this code. Running Java2D.jar in Xcode asserts here >> with this message: >> >> 2021-02-11 14:11:45.710457-0600 java[49971:9486360] Metal API Validation >> Enabled >> 2021-02-11 14:11:46.038720-0600 system_profiler[49975:9486885] Metal API >> Validation Enabled >> -[MTLDebugBlitCommandEncoder >> internalValidateCopyFromTexture:sourceSlice:sourceLevel:sourceOrigin:sourceSize:toTexture:destinationSlice:destinationLevel:destinationOrigin:options:]:474: >> failed assertion `(sourceOrigin.y + sourceSize.height)(444) must be <= >> height(400).' >> (lldb) >> >> and forcing the execution past it results in the java process crashing. >> >> A solution would be to clip the src size like this: >> >> NSUInteger src_x = self.leftInset*self.contentsScale; >> NSUInteger src_y = self.topInset*self.contentsScale; >> NSUInteger src_w = self.buffer.width-src_x; >> NSUInteger src_h = self.buffer.height-src_y; >> >> [blitEncoder >> copyFromTexture:self.buffer sourceSlice:0 sourceLevel:0 >> sourceOrigin:MTLOriginMake(src_x, src_y, 0) >> sourceSize:MTLSizeMake(src_w, src_h, 1) >> toTexture:mtlDrawable.texture destinationSlice:0 >> destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; >> [blitEncoder endEncoding]; > > I guess you will only see this if `Metal API Validation Enabled` Which actually begs a question of whether we tested with `Metal API Validation Enabled` ? - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Thu, 11 Feb 2021 20:55:35 GMT, Gerard Ziemski wrote: >> Ajit Ghaisas has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Lanai PR#181 - 8261143 - aghaisas >> - Lanai PR#180 - 8261546 - jdv > > src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 112: > >> 110: sourceSize:MTLSizeMake(self.buffer.width, >> self.buffer.height, 1) >> 111: toTexture:mtlDrawable.texture destinationSlice:0 >> destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; >> 112: [blitEncoder endEncoding]; > > There is an issue with this code. Running Java2D.jar in Xcode asserts here > with this message: > > 2021-02-11 14:11:45.710457-0600 java[49971:9486360] Metal API Validation > Enabled > 2021-02-11 14:11:46.038720-0600 system_profiler[49975:9486885] Metal API > Validation Enabled > -[MTLDebugBlitCommandEncoder > internalValidateCopyFromTexture:sourceSlice:sourceLevel:sourceOrigin:sourceSize:toTexture:destinationSlice:destinationLevel:destinationOrigin:options:]:474: > failed assertion `(sourceOrigin.y + sourceSize.height)(444) must be <= > height(400).' > (lldb) > > and forcing the execution past it results in the java process crashing. > > A solution would be to clip the src size like this: > > NSUInteger src_x = self.leftInset*self.contentsScale; > NSUInteger src_y = self.topInset*self.contentsScale; > NSUInteger src_w = self.buffer.width-src_x; > NSUInteger src_h = self.buffer.height-src_y; > > [blitEncoder > copyFromTexture:self.buffer sourceSlice:0 sourceLevel:0 > sourceOrigin:MTLOriginMake((jint)src_x, (jint)src_y, 0) > sourceSize:MTLSizeMake(src_w, src_h, 1) > toTexture:mtlDrawable.texture destinationSlice:0 > destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; > [blitEncoder endEncoding]; I guess you will only see this if `Metal API Validation Enabled` - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Thu, 11 Feb 2021 11:51:57 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#181 - 8261143 - aghaisas > - Lanai PR#180 - 8261546 - jdv Changes requested by gziemski (Committer). src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 112: > 110: sourceSize:MTLSizeMake(self.buffer.width, > self.buffer.height, 1) > 111: toTexture:mtlDrawable.texture destinationSlice:0 > destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; > 112: [blitEncoder endEncoding]; There is an issue with this code. Running Java2D.jar in Xcode asserts here with this message: 2021-02-11 14:11:45.710457-0600 java[49971:9486360] Metal API Validation Enabled 2021-02-11 14:11:46.038720-0600 system_profiler[49975:9486885] Metal API Validation Enabled -[MTLDebugBlitCommandEncoder internalValidateCopyFromTexture:sourceSlice:sourceLevel:sourceOrigin:sourceSize:toTexture:destinationSlice:destinationLevel:destinationOrigin:options:]:474: failed assertion `(sourceOrigin.y + sourceSize.height)(444) must be <= height(400).' (lldb) and forcing the execution past it results in the java process crashing. A solution would be to clip the src size like this: NSUInteger src_x = self.leftInset*self.contentsScale; NSUInteger src_y = self.topInset*self.contentsScale; NSUInteger src_w = self.buffer.width-src_x; NSUInteger src_h = self.buffer.height-src_y; [blitEncoder copyFromTexture:self.buffer sourceSlice:0 sourceLevel:0 sourceOrigin:MTLOriginMake((jint)src_x, (jint)src_y, 0) sourceSize:MTLSizeMake(src_w, src_h, 1) toTexture:mtlDrawable.texture destinationSlice:0 destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; [blitEncoder endEncoding]; - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: 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: 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: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Wed, 10 Feb 2021 21:40:46 GMT, Gerard Ziemski wrote: >> Changes requested by gziemski (Committer). > > 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! 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: https://user-images.githubusercontent.com/63425797/107575744-dc0ca180-6bb5-11eb-9eb3-5cff415eb8a3.png;> Metal (the green color has a yellowish tint): https://user-images.githubusercontent.com/63425797/107575716-cf884900-6bb5-11eb-8ae8-14212ec94e87.png;> Performance wise I did not see much difference either way. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 23:07:39 GMT, Gerard Ziemski wrote: >> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Lanai PR#175 - 8261304 - aghaisas > > Changes requested by gziemski (Committer). I tried to code review the native implementation file, 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! - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 12:28:07 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)
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Lanai PR#175 - 8261304 - aghaisas
Changes requested by gziemski (Committer).
src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 61:
> 59:
> 60: public abstract class MTLSurfaceData extends SurfaceData
> 61: implements AccelSurface {
`MTLSurfaceData` and `OGLSurfaceData` seem to share lots of the same code,
isn't there a way to refactor the common code out?
There are other files that structually look identical, except for the names of
classes they use, ex `MTLMaskBlit.java` and `OGLMaskBlit.java`.
-
PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 12:28:07 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)
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Lanai PR#175 - 8261304 - aghaisas
> > General comment - I am not sure I like the `MTL` prefix acronym in names
> > (ex. `sun.java2d.metal.MTLVolatileSurfaceManager`).
> > I think you tried to match the `CGL`, but that is a real acronym that
> > stands for "Core Graphics Layer" (I think).
> > `MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I
> > suppose, but also just `Metal` would work just fine.
>
> `MTL` is the abbreviation that Apple uses for Metal in all of their APIs. The
> only potential issue I might see with this prefix is in the native code where
> there could be name collisions between Java2D's names and Apple's names.
> Since we haven't run into such a collision, I don't think this needs to
> change, and wouldn't necessarily affect the Java class names anyway. If we
> were to consider it, `METAL` seems better than `ML` (which is too short to be
> descriptive and might suggest machine learning).
If Apple itself uses `MTL` then we are good.
src/java.desktop/macosx/classes/sun/awt/CGraphicsConfig.java line 35:
> 33:
> 34: import sun.java2d.SurfaceData;
> 35: import sun.java2d.opengl.CGLLayer;
Not needed import anymore?
src/java.desktop/macosx/classes/sun/awt/CGraphicsDevice.java line 113:
> 111: // This indicates fallback to other rendering pipeline also
> failed.
> 112: // Should never reach here
> 113: throw new InternalError("Error - unable to initialize any
> rendering pipeline.");
There is no software based renderer to fall back here?
src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 66:
> 64:propString.equals("f") ||
> 65:propString.equals("False") ||
> 66:propString.equals("F"))
Shouldn't `1` and `0` be also allowed here?
src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 100:
> 98: oglState = PropertyState.ENABLED; // Enable
> default pipeline
> 99: metalState = PropertyState.DISABLED; // Disable
> non-default pipeline
> 100: }
This matches JEP 382 specification, but even when both GL is `false` and Metal
is `false` we still get GL? There is no software based pipeline anymore?
src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 131:
> 129: @Native
> 130: static final int CAPS_EXT_GRAD_SHADER = (FIRST_PRIVATE_CAP <<
> 3);
> 131: /** Indicates the presence of the GL_ARB_texture_rectangle
> extension. */
Reference to `GL_ARB_texture_rectangle` extension in Metal pipeline?
src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 134:
> 132: @Native
> 133: static final int CAPS_EXT_TEXRECT = (FIRST_PRIVATE_CAP <<
> 4);
> 134: /** Indicates the presence of the GL_NV_texture_barrier
> extension. */
Reference to `GL_NV_texture_barrier` extension in Metal pipeline?
src/java.desktop/macosx/classes/sun/java2d/metal/MTLRenderQueue.java line 97:
> 95: public static void disposeGraphicsConfig(long pConfigInfo) {
> 96: MTLRenderQueue rq = getInstance();
> 97: rq.lock();
Is it allowed to have multiple `MTLRenderQueue` instances?
If not, then I see this pattern everywhere:
MTLRenderQueue rq = getInstance();
rq.lock();
{
...
}
rq.unlock();
why not just do:
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 17:15:25 GMT, Gerard Ziemski wrote: >> The file in `RenderPerfTest` should have a GPLv2 license header (no >> Classpath). I filed >> [JDK-8261273](https://bugs.openjdk.java.net/browse/JDK-8261273) and also >> highlighted a couple examples below. > > General comment - I am not sure I like the `MTL` prefix acronym in names (ex. > `sun.java2d.metal.MTLVolatileSurfaceManager`). > > I think you tried to match the `CGL`, but that is a real acronym that stands > for "Core Graphics Layer" (I think). > > `MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I > suppose, but also just `Metal` would work just fine. I'm in the process of reviewing this feature, but there is lots of code to go through - please wait for my review. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Sat, 6 Feb 2021 00:53:08 GMT, Kevin Rushforth wrote: >> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Lanai PR#175 - 8261304 - aghaisas > > The file in `RenderPerfTest` should have a GPLv2 license header (no > Classpath). I filed > [JDK-8261273](https://bugs.openjdk.java.net/browse/JDK-8261273) and also > highlighted a couple examples below. General comment - I am not sure I like the `MTL` prefix acronym in names (ex. `sun.java2d.metal.MTLVolatileSurfaceManager`). I think you tried to match the `CGL`, but that is a real acronym that stands for "Core Graphics Layer" (I think). `MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I suppose, but also just `Metal` would work just fine. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Fri, 29 Jan 2021 19:53:32 GMT, Phil Race wrote: >> src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 83: >> >>> 81: stringWithFileSystemRepresentation:chs length:len]; >>> 82: return result; >>> 83: } >> >> Why are we doing: >> >> `java_string -> chars -> NSString -> chars -> [NSFileManager >> stringWithFileSystemRepresentation]` >> >> ? >> >> Why not just get the raw characters form JNI and feed them directly into >> `[NSFileManager stringWithFileSystemRepresentation]`, ie: >> >> `java_string -> chars -> [NSFileManager stringWithFileSystemRepresentation]` >> >> and skip the `JavaStringToNSString` step altogether? > > I tried to explain the odd-ness of this in the comments preceding the > function definition. > Near as I could figure out that intermediate call is needed to do the > decomposition since the NSFileManager won't do that. > But this is not exactly my area of expertise and there may be a better way. > Who is an expert on the intricacies of the macOS file system naming ? How about a comment like: /* Returns an NSString in decomposed UTF16 format that is compatible with HFS's expectation of the UTF16 format for file system paths. Example string: "/Users/Amélie/" Java's UTF16 string is "/ U s e r s / A m \351 l i e /" macOS UTF16 string suitable for HFS is "/ U s e r s / A m e \314 \201 l i e /" There is no direct API that takes in NSString UTF16 encoded by Java and produces NSString UTF16 for HFS, so we first need to decompose it into chars (suitable for low level C file APIs), and only then create NSString representation of this decomposition back into UTF16 string. */ ? I borrowed the API description from Apple's original `JNFNormalizedNSStringForPath` API, but added an example and different description. - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Mon, 1 Feb 2021 18:31:23 GMT, Gerard Ziemski wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8260616: Removing remaining JNF dependencies in the java.desktop modul > > Marked as reviewed by gziemski (Committer). The changes look good to me, but please see my comment from my review about documenting `NormalizedPathNSStringFromJavaString()` API. - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Fri, 29 Jan 2021 17:24:05 GMT, Phil Race wrote: >> This completes the desktop module JNF removal >> >> * remove -framework JavaNativeFoundation from make files >> >> * remove #import from all >> source files. If needed add import of JNIUtilities.h to get jni.h >> definitions - better anyway since then it gets the current JDK ones not the >> ones from the O/S >> >> * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString >> with JavaStringToNSString >> >> * replace JNFNormalizedNSStringForPath with >> NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with >> NormalizedPathJavaStringFromNSString >> >> * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI >> >> * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the >> vast majority already did this) >> >> * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject >> perform* methods. >> >> * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and >> use where needed. >> >> * Remove the single usage of JNFPerformEnvBlock >> >> * replace JNFJavaToNSNumber in single A11Y file with local replacement >> >> * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with >> local replacement >> >> * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m >> >> * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8260616: Removing remaining JNF dependencies in the java.desktop modul Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module
On Fri, 29 Jan 2021 00:30:21 GMT, Phil Race wrote:
> This completes the desktop module JNF removal
>
> * remove -framework JavaNativeFoundation from make files
>
> * remove #import from all
> source files. If needed add import of JNIUtilities.h to get jni.h definitions
> - better anyway since then it gets the current JDK ones not the ones from the
> O/S
>
> * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString
> with JavaStringToNSString
>
> * replace JNFNormalizedNSStringForPath with
> NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with
> NormalizedPathJavaStringFromNSString
>
> * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI
>
> * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast
> majority already did this)
>
> * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject
> perform* methods.
>
> * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and
> use where needed.
>
> * Remove the single usage of JNFPerformEnvBlock
>
> * replace JNFJavaToNSNumber in single A11Y file with local replacement
>
> * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with
> local replacement
>
> * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m
>
> * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION)
Changes requested by gziemski (Committer).
src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 608:
> 606: {
> 607: // Get string to draw and the length
> 608: const jchar *unichars = JNFGetStringUTF16UniChars(env, str);
According to `JNFString.h`, the `JNFGetStringUTF16UniChars()` API:
/*
* Gets UTF16 unichars from a Java string, and checks for errors and raises a
JNFException if
* the unichars cannot be obtained from Java.
*/
raises a (JNFException) if it can't get the chars, but now we simply return if
`GetStringChars()` can not return the chars.
Seems like we handle this case differently in the new code now - is this change
desired and correct?
src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 601:
> 599: jchar unichars[len];
> 600: (*env)->GetStringRegion(env, str, 0, len, unichars);
> 601: CHECK_EXCEPTION();
Are `JNF_CHECK_AND_RETHROW_EXCEPTION` and `CHECK_EXCEPTION` equivalent?
`JNF_CHECK_AND_RETHROW_EXCEPTION` seems to throw exception, but
`CHECK_EXCEPTION` does not?
src/java.desktop/macosx/native/libosxui/ScreenMenu.m line 165:
> 163: */
> 164: static jlong NSTimeIntervalToJavaMilliseconds(NSTimeInterval interval) {
> 165: NSTimeInterval interval1970 = interval + NSTimeIntervalSince1970;
Is it required for the APIs using the value from this function to expect the
time calculated since Jan 1st 1970?
src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 83:
> 81: stringWithFileSystemRepresentation:chs length:len];
> 82: return result;
> 83: }
Why are we doing:
`java_string -> chars -> NSString -> chars -> [NSFileManager
stringWithFileSystemRepresentation]`
?
Why not just get the raw characters form JNI and feed them directly into
`[NSFileManager stringWithFileSystemRepresentation]`, ie:
`java_string -> chars -> [NSFileManager stringWithFileSystemRepresentation]`
and skip the `JavaStringToNSString` step altogether?
-
PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8257853: Remove dependencies on JNF's JNI utility functions in AWT and 2D code [v6]
On Fri, 18 Dec 2020 11:20:23 GMT, Prasanta Sadhukhan wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8257853: Remove dependencies on JNF's JNI utility functions in AWT and 2D >> code > > I believe we could add CHECK_EXCEPTION in few places...pointing those out... There seems to be a pattern where we replace `JNFCallObjectMethod()` with a pair of `CallObjectMethod()` immediately followed by `CHECK_EXCEPTION()` There are places where currently in this fix, this pattern is missing the `CHECK_EXCEPTION()` Can we replace `JNFCallObjectMethod()` by a single function call (or macro) that wraps both `CallObjectMethod()` and `CHECK_EXCEPTION()` into one? This would make this change more robust and make it easier to review it. - PR: https://git.openjdk.java.net/jdk/pull/1679
Re: RFR: 8257853: Remove dependencies on JNF's JNI utility functions in AWT and 2D code [v5]
On Mon, 14 Dec 2020 21:31:15 GMT, Phil Race wrote: >> This defines some macros to support declaring and initialising statically >> allocated instances of jclass, jmethodID and jfieldID >> and changes many existing uses of JNF macros/functions to use these instead. >> Then calls to JNFCall* and JNFNewObject - etc are updated to directly call >> JNI methods >> JNI exception checking macros are added as needed. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8257853: Remove dependencies on JNF's JNI utility functions in AWT and 2D > code src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m line 338: > 336: } > 337: > 338: sAccessibilityClass = (*env)->CallStaticObjectMethod(env, > sjc_CAccessibility, jm_getAccessibility, result); // AWT_THREADING Safe > (known object) Do we need `CHECK_EXCEPTION();` after `CallStaticObjectMethod ` here? - PR: https://git.openjdk.java.net/jdk/pull/1679
Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"
Thank you for the review. I'm happy to wait for your fix to go in first - this will make my fix smaller. cheers On 11/13/19 2:05 PM, Langer, Christoph wrote: Hi Gerard, generally it looks like a nice cleanup. I've got a patch prepared though, which I was planning on posting tomorrow. It is about cleanup for the canonicalize function in libjava. I wanted to use jdk_util.h for the function prototype. I had not yet filed a bug but here is what I have: http://cr.openjdk.java.net/~clanger/webrevs/cleanup-canonicalize/ So maybe you could refrain from removing jdk_util.h or maybe you can hold off submitting your change until my cleanup is reviewed? I'll create a bug and post an official review thread tomorrow... Thanks Christoph -Original Message- From: hotspot-dev On Behalf Of Mandy Chung Sent: Mittwoch, 13. November 2019 20:30 To: gerard ziemski Cc: awt-dev@openjdk.java.net; hotspot-dev developers ; core-libs-...@openjdk.java.net Subject: Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code" On 11/13/19 10:50 AM, gerard ziemski wrote: Hi all, Please review this cleanup, where we remove JDK_GetVersionInfo0 and related code, since we can get build versions directly from within the VM itself: I'm including core-libs and awt in this review because the proposed fix touches their corresponding files. bug: https://bugs.openjdk.java.net/browse/JDK-8223261 webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1 tests: passes Mach5 tier1,2,3,4,5,6 This is a good clean up. JDK_GetVersionInfo0 was needed long time ago in particular in HS express support that is no longer applicable. One leftover comment should also be removed. src/hotspot/share/runtime/vm_version.hpp // Gets the jvm_version_info.jvm_version defined in jvm.h otherwise looks good. Mandy
RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"
Hi all, Please review this cleanup, where we remove JDK_GetVersionInfo0 and related code, since we can get build versions directly from within the VM itself: I'm including core-libs and awt in this review because the proposed fix touches their corresponding files. bug: https://bugs.openjdk.java.net/browse/JDK-8223261 webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1 tests: passes Mach5 tier1,2,3,4,5,6 cheers
Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"
Thank you for the review, will remove the comment and updated the webrev, but only after Christop Langer gets his fix in - I'm going to wait for him to check in first. cheers On 11/13/19 1:29 PM, Mandy Chung wrote: On 11/13/19 10:50 AM, gerard ziemski wrote: Hi all, Please review this cleanup, where we remove JDK_GetVersionInfo0 and related code, since we can get build versions directly from within the VM itself: I'm including core-libs and awt in this review because the proposed fix touches their corresponding files. bug: https://bugs.openjdk.java.net/browse/JDK-8223261 webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1 tests: passes Mach5 tier1,2,3,4,5,6 This is a good clean up. JDK_GetVersionInfo0 was needed long time ago in particular in HS express support that is no longer applicable. One leftover comment should also be removed. src/hotspot/share/runtime/vm_version.hpp // Gets the jvm_version_info.jvm_version defined in jvm.h otherwise looks good. Mandy
