Hello All, Thanks for the review. Changes are merged into the sandbox.
Made some more changes in sandbox like JDK-8226560 <https://bugs.openjdk.java.net/browse/JDK-8226560> and took mainline changes into our metal-prototype-branch. We can take latest change from this sandbox branch and work further. Thanks, Jay > On 21-Jun-2019, at 2:21 AM, Philip Race <philip.r...@oracle.com> wrote: > > I'm OK with this. We will doubtless incrementally revisit everything before > we are done. > > -phil. > > On 6/20/19, 12:52 PM, Kevin Rushforth wrote: >> >> As long as you've addressed all Phil's concerns, it looks OK to me. >> >> -- Kevin >> >> On 6/20/2019 5:57 AM, Jayathirth Rao wrote: >>> 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/%7Ejdv/8225160/webrev.02/> >>> >>> Regards, >>> Jay >>> >>>> On 20-Jun-2019, at 12:00 AM, Alexey Ushakov <alexey.usha...@jetbrains.com >>>> <mailto: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/%7Ejdv/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/%7Ejdv/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/%7Ejdv/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/%7Ejdv/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/%7Ejdv/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/%7Ejdv/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/%7Ejdv/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/%7Ejdv/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 >>>>> >>>> >>> >>
