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

Philip Race philip.race at oracle.com
Thu Jun 20 20:51:18 UTC 2019 

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.ushakov at jetbrains.com <mailto:alexey.ushakov at 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.d.v at oracle.com 
>>>> <mailto:jayathirth.d.v at 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.race at oracle.com 
>>>>> <mailto:philip.race at 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
>>>>>
>>>>> +        -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/20190620/dcdfb6e3/attachment-0001.html>

More information about the 2d-dev mailing list