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

Reply via email to