Hello Jay and Phil, This changes looks good for me.
> 5) Time tracing for primitive drawing was present in JetBrain’s code so we > took it. It may hamper overall performance while drawing, if it is not needed > we can remove it. Concerning the logging we can remove it. The junit microbenchmarks are enough for now to measure and compare performance of drawing. Best Regards, Alexey > On 19 Jun 2019, at 12:08, Jayathirth Rao <jayathirth....@oracle.com> wrote: > > Hi Phil, > > Thanks for your inputs. > 1) Removed MetalKit reference in make file > 2) Removed reference to sun.java2d.metal in jdk8_packages.dat file. > 3) For pipeline selection logic, I have created new sub-task JDK-8226384 > <https://bugs.openjdk.java.net/browse/JDK-8226384> > 4) Removed MetalKit references in imports which are not needed. > 5) Time tracing for primitive drawing was present in JetBrain’s code so we > took it. It may hamper overall performance while drawing, if it is not needed > we can remove it. > > Please find updated webrev for review: > http://cr.openjdk.java.net/~jdv/8225160/webrev.01/ > <http://cr.openjdk.java.net/~jdv/8225160/webrev.01/> > > For file comparison of new and old files, I have created comparison table at > http://cr.openjdk.java.net/~jdv/8225160/file_compare.rtf > <http://cr.openjdk.java.net/~jdv/8225160/file_compare.rtf>. > > Thanks, > Jay > >> On 19-Jun-2019, at 6:01 AM, Philip Race <philip.r...@oracle.com >> <mailto:philip.r...@oracle.com>> wrote: >> >> >> 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 >> >> <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 >> >> <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 >> >> <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 >> >> <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/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 >> >> <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 >>> <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 >>> <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 >>> <https://bugs.openjdk.java.net/browse/JDK-8225165> . >>> >>> Thanks, >>> Jay >
