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

Kevin Rushforth kevin.rushforth at oracle.com
Thu Jun 20 19:52:00 UTC 2019 

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/
>
> 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/
>>>
>>> For file comparison of new and old files, I have created comparison 
>>> table at http://cr.openjdk.java.net/~jdv/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/a0418627/attachment-0001.html>

More information about the 2d-dev mailing list