Hi Alexey, Thanks for the review.
Removed new trace definitions and there references. Webrev without traces : http://cr.openjdk.java.net/~jdv/8225160/webrev.02/ <http://cr.openjdk.java.net/~jdv/8225160/webrev.02/> Regards, Jay > On 20-Jun-2019, at 12:00 AM, Alexey Ushakov <alexey.usha...@jetbrains.com> > wrote: > > 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 >> <mailto: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 >> >
