[OpenJDK 2D-Dev] RFR 8225160: Merge JDK-8220154 initial metal implementation patch to the jdk sandbox branch
Jayathirth Rao
jayathirth.d.v at oracle.com
Fri Jun 21 09:17:39 UTC 2019
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.race at 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.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 <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
>>>>>
>>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20190621/8b760df4/attachment-0001.html>
More information about the 2d-dev
mailing list