[OpenJDK 2D-Dev] RFR 8225160: Merge JDK-8220154 initial metal implementation patch to the jdk sandbox branch
Alexey Ushakov
alexey.ushakov at jetbrains.com
Wed Jun 19 18:30:04 UTC 2019
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> 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/20190619/b789f1db/attachment.html>
More information about the 2d-dev
mailing list