<AWT Dev> RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction

Ajit Ghaisas-2
Refer JBS for 3 issues that this PR addresses.
In addition, I have corrected an erroneous free() call in the same method I was cleaning up.

-------------

Commit messages:
 - 8263363 - cleanup

Changes: https://git.openjdk.java.net/jdk/pull/3357/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3357&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263363
  Stats: 21 lines in 3 files changed: 1 ins; 16 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3357.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3357/head:pull/3357

PR: https://git.openjdk.java.net/jdk/pull/3357
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction

Sergey Bylokhov-2
On Tue, 6 Apr 2021 14:12:55 GMT, Ajit Ghaisas <[hidden email]> wrote:

> Refer JBS for 3 issues that this PR addresses.
> In addition, I have corrected an erroneous free() call in the same method I was cleaning up.

src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java line 149:

> 147:         try {
> 148:             // getMTLConfigInfo() creates new MTLContext, so we should first
> 149:             // invalidate the current Java-level context and flush the queue...

The old discussion was related not only to the comment but to the invalidateCurrentContext, do we need to do it?

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m line 152:

> 150:     NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
> 151:
> 152:

Please also check how this function is called, looks like previously it was called as a selector+an array as a parameter, and then reworked as a performOnMainThreadWaiting+block, but it still use an array as a parameter. I think it is similar to JDK-8238075.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3357
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v2]

Ajit Ghaisas-2
In reply to this post by Ajit Ghaisas-2
> Refer JBS for 3 issues that this PR addresses.
> In addition, I have corrected an erroneous free() call in the same method I was cleaning up.

Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:

  Review fixes

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3357/files
  - new: https://git.openjdk.java.net/jdk/pull/3357/files/ade2381d..36ed087b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3357&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3357&range=00-01

  Stats: 128 lines in 1 file changed: 20 ins; 81 del; 27 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3357.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3357/head:pull/3357

PR: https://git.openjdk.java.net/jdk/pull/3357
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v2]

Ajit Ghaisas-2
In reply to this post by Sergey Bylokhov-2
On Wed, 7 Apr 2021 02:50:03 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Review fixes
>
> src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java line 149:
>
>> 147:         try {
>> 148:             // getMTLConfigInfo() creates new MTLContext, so we should first
>> 149:             // invalidate the current Java-level context and flush the queue...
>
> The old discussion was related not only to the comment but to the invalidateCurrentContext, do we need to do it?

This is the only place where we use MTLContext.invalidateCurrentContext() - which when processed in MTLRenderQueue - clears some native stuff and nulls out both mtlc and dstOps pointers maintained in MTLRenderQueue.m. I think, this will be important once we get rid of SET_SCRATCH_SURFACE under JDK-8263309.

> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m line 152:
>
>> 150:     NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
>> 151:
>> 152:
>
> Please also check how this function is called, looks like previously it was called as a selector+an array as a parameter, and then reworked as a performOnMainThreadWaiting+block, but it still use an array as a parameter. I think it is similar to JDK-8238075.

Excellent point! Thanks for the pointer to the bug.
A lot of code in this file can be cleaned up. I will update the PR soon.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3357
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v3]

Ajit Ghaisas-2
In reply to this post by Ajit Ghaisas-2
> Refer JBS for 3 issues that this PR addresses.
> In addition, I have corrected an erroneous free() call in the same method I was cleaning up.

Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:

  log message correction

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3357/files
  - new: https://git.openjdk.java.net/jdk/pull/3357/files/36ed087b..45e7aec0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3357&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3357&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3357.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3357/head:pull/3357

PR: https://git.openjdk.java.net/jdk/pull/3357
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v3]

Sergey Bylokhov-2
In reply to this post by Ajit Ghaisas-2
On Thu, 8 Apr 2021 10:39:38 GMT, Ajit Ghaisas <[hidden email]> wrote:

>> src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java line 149:
>>
>>> 147:         try {
>>> 148:             // getMTLConfigInfo() creates new MTLContext, so we should first
>>> 149:             // invalidate the current Java-level context and flush the queue...
>>
>> The old discussion was related not only to the comment but to the invalidateCurrentContext, do we need to do it?
>
> This is the only place where we use MTLContext.invalidateCurrentContext() - which when processed in MTLRenderQueue - clears some native stuff and nulls out both mtlc and dstOps pointers maintained in MTLRenderQueue.m. I think, this will be important once we get rid of SET_SCRATCH_SURFACE under JDK-8263309.

But why you need to invalidate context here? Why do you need "clears some native stuff and nulls out both mtlc and dstOps pointers maintained in MTLRenderQueue.m"?

In OGL the getCGLConfigInfo() change the state of the OGL state due to "makeCurrentContext", this is why we need to update the lava level state to "invalid", otherwise we will get a mismatch between the state in the native and java state.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3357