[OpenJDK 2D-Dev] RFR 8225160: Merge JDK-8220154 initial metal implementation patch to the jdk sandbox branch

[Philip Race]

Hi,

First
These comments are limited to the files that are *modified* and don't
address anything where you have deleted sandbox files and replaced them by JB files.
And that is very hard to "diff" anyway .. but it is the biggest batch so I am not
sure how to address it.
Can you give a detailed explanation of what is in the "new" files vs the "old" files
and how you chose one over the other ?

So this is my quick take on just the modified files :
  
http://cr.openjdk.java.net/~jdv/8225160/webrev.00/make/lib/Awt2dLibraries.gmk.udiff.html

+        -framework Metal \
+        -framework MetalKit \

I thought there were no dependencies on MetalKit ?

http://cr.openjdk.java.net/~jdv/8225160/webrev.00/src/java.base/share/classes/jdk/internal/module/jdk8_packages.dat.udiff.html

+sun.java2d.metal

I am sure this is wrong. That file is a list of internal packages
that were present in JDK 8

http://cr.openjdk.java.net/~jdv/8225160/webrev.00/src/java.desktop/macosx/classes/sun/java2d/MacosxSurfaceManagerFactory.java.udiff.html

JFYI I am not sure I like *either* of the pieces of code here.
The deleted one or the new one.
By which I mean the logic of
if (metal) {
   return MetalSM()
} else {
   return CGLSM();
}

both here, and where similar logic appears in other files.
We should be installing something that always returns what we need based
on choice of pipeline at start up, not doing error prone repeating of 
the decision.
But that is something to discuss later since it isn't a merge issue.

http://cr.openjdk.java.net/~jdv/8225160/webrev.00/src/java.desktop/macosx/native/libawt_lwawt/awt/AWTView.h.udiff.html

+#import<Metal/Metal.h>
+#import<MetalKit/MetalKit.h>

I am failing to locate what needs these new imports ??

Same here
http://cr.openjdk.java.net/~jdv/8225160/webrev.00/src/java.desktop/macosx/native/libawt_lwawt/awt/CGraphicsDevice.m.udiff.html

http://cr.openjdk.java.net/~jdv/8225160/webrev.00/src/java.desktop/share/classes/sun/java2d/loops/Blit.java.udiff.html

I see this pattern being added to the Trace* classes.
At first it wasn't apparent from the diff it is the Trace classes,
so this is something we can discuss but it may need to be measured
to see if it corrupts the very data you want.

          {
+            if ((traceflags&  TRACEPTIME) == 0) {
              tracePrimitive(target);
+            }
+            long time = System.nanoTime();
              target.Blit(src, dst, comp, clip,
                          srcx, srcy, dstx, dsty, width, height);
+            tracePrimitiveTime(target, System.nanoTime() - time);


-phil



On 6/17/19, 2:19 AM, Jayathirth Rao wrote:
> Hello All,
>
> Please review the below code changes:
>
> JBS : https://bugs.openjdk.java.net/browse/JDK-8225160
>
> We have merged most of the code provided for review by JetBrains for 
> JDK-8220154 <https://bugs.openjdk.java.net/browse/JDK-8220154> to the 
> jdk/sandbox(http://hg.openjdk.java.net/jdk/sandbox).
> Corresponding webrev for the change is 
> http://cr.openjdk.java.net/~jdv/8225160/webrev.00/ 
> <http://cr.openjdk.java.net/%7Ejdv/8225160/webrev.00/> . This 
> webrev has JetBrains code from JDK-8220154 
> <https://bugs.openjdk.java.net/browse/JDK-8220154> and our delta 
> modification[1] over jdk/sandbox metal-prototype-branch
>
> For additional info :
> 1. A webrev relative to the jdk/client codebase - this has JetBrains 
> webrev for JDK-8220154 
> <https://bugs.openjdk.java.net/browse/JDK-8220154> and our delta 
> modification
> http://cr.openjdk.java.net/~aghaisas/8225160/JB_plus_delta/webrev.1/ 
> <http://cr.openjdk.java.net/%7Eaghaisas/8225160/JB_plus_delta/webrev.1/>
>
> 2. A webrev showing our delta modifications over JetBrains webrev for 
> JDK-8220154 <https://bugs.openjdk.java.net/browse/JDK-8220154>
> http://cr.openjdk.java.net/~aghaisas/8225160/webrev.1/ 
> <http://cr.openjdk.java.net/%7Eaghaisas/8225160/webrev.1/>
>
>
> [1] Delta modification: At native rendering side Ajit has added delta 
> modifications to implement FillRect and DrawParallelogram logic using 
> JetBrains initialisation logic.
>
> Apart from how we initialise GraphicsConfig and native rendering most 
> of the upper level logic is similar so we have taken most of the 
> JetBrains code with MTL*** files. Also native rendering state 
> management code from JetBrains we have merged.
> Comparison of code at GraphicsConfig, SurfaceData and Layer is 
> captured under subtask : 
> https://bugs.openjdk.java.net/browse/JDK-8225165 .
>
> Thanks,
> Jay
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20190618/11425861/attachment.html>