[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
Thu Jun 20 12:57:07 UTC 2019
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.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/~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.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/~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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20190620/980d5324/attachment.html>
More information about the 2d-dev
mailing list