<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Phil,<div class=""><br class=""></div><div class="">Thanks for your inputs.</div><div class="">1) Removed MetalKit reference in make file</div><div class="">2) Removed reference to sun.java2d.metal in jdk8_packages.dat file.</div><div class="">3) For pipeline selection logic, I have created new sub-task <a class="issue-link" data-issue-key="JDK-8226384" href="https://bugs.openjdk.java.net/browse/JDK-8226384" id="key-val" rel="4993674">JDK-8226384</a></div><div class="">4) Removed MetalKit references in imports which are not needed.<br class=""><div>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.</div><div><br class=""></div><div>Please find updated webrev for review:</div><div><a href="http://cr.openjdk.java.net/~jdv/8225160/webrev.01/" class="">http://cr.openjdk.java.net/~jdv/8225160/webrev.01/</a> </div><div><br class=""></div><div>For file comparison of new and old files, I have created comparison table at <a href="http://cr.openjdk.java.net/~jdv/8225160/file_compare.rtf" class="">http://cr.openjdk.java.net/~jdv/8225160/file_compare.rtf</a>. </div><div><br class=""></div><div>Thanks,</div><div>Jay</div><div><br class=""><blockquote type="cite" class=""><div class="">On 19-Jun-2019, at 6:01 AM, Philip Race <<a href="mailto:philip.race@oracle.com" class="">philip.race@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">
<meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1" class="">
<pre class=""><span class="new">
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 :
<a class="moz-txt-link-freetext" href="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</a>
+ -framework Metal \</span>
<span class="new">+ -framework MetalKit \</span></pre>
I thought there were no dependencies on MetalKit ?<br class="">
<br class="">
<a class="moz-txt-link-freetext" href="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</a><br class="">
<br class="">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1" class="">
<pre class=""><span class="new">+sun.java2d.metal
I am sure this is wrong. That file is a list of internal packages
that were present in JDK 8
</span><a class="moz-txt-link-freetext" href="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</a>
</pre>
JFYI I am not sure I like *either* of the pieces of code here.<br class="">
The deleted one or the new one.<br class="">
By which I mean the logic of<br class="">
if (metal) {<br class="">
return MetalSM()<br class="">
} else {<br class="">
return CGLSM(); <br class="">
}<br class="">
<br class="">
both here, and where similar logic appears in other files.<br class="">
We should be installing something that always returns what we need
based<br class="">
on choice of pipeline at start up, not doing error prone repeating
of the decision.<br class="">
But that is something to discuss later since it isn't a merge issue.<br class="">
<br class="">
<a class="moz-txt-link-freetext" href="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</a><br class="">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1" class="">
<pre class=""><span class="new">+#import <Metal/Metal.h></span>
<span class="new">+#import <MetalKit/MetalKit.h></span></pre>
I am failing to locate what needs these new imports ??<br class="">
<br class="">
Same here<br class="">
<a class="moz-txt-link-freetext" href="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</a><br class="">
<br class="">
<a class="moz-txt-link-freetext" href="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</a><br class="">
<br class="">
I see this pattern being added to the Trace* classes.<br class="">
At first it wasn't apparent from the diff it is the Trace classes,<br class="">
so this is something we can discuss but it may need to be measured<br class="">
to see if it corrupts the very data you want.<br class="">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1" class="">
<pre class=""> {
<span class="new">+ if ((traceflags & TRACEPTIME) == 0) {</span>
tracePrimitive(target);
<span class="new">+ }</span>
<span class="new">+ long time = System.nanoTime();</span>
target.Blit(src, dst, comp, clip,
srcx, srcy, dstx, dsty, width, height);
<span class="new">+ tracePrimitiveTime(target, System.nanoTime() - time);
-phil
</span></pre>
<br class="">
<br class="">
On 6/17/19, 2:19 AM, Jayathirth Rao wrote:
<blockquote cite="mid:F2E4B1E8-250E-42F8-9174-335ACB9F156A@oracle.com" type="cite" class="">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1" class="">
Hello All,
<div class=""><br class="">
</div>
<div class="">Please review the below code changes: </div>
<div class=""><br class="">
</div>
<div class="">JBS : <a moz-do-not-send="true" href="https://bugs.openjdk.java.net/browse/JDK-8225160" class="">https://bugs.openjdk.java.net/browse/JDK-8225160</a> </div>
<div class=""><br class="">
</div>
We have merged most of the code provided for review by JetBrains
for <a moz-do-not-send="true" href="https://bugs.openjdk.java.net/browse/JDK-8220154" title="Improve java2d rendering performance on macOS by using
Metal framework" class="issue-link" data-issue-key="JDK-8220154">JDK-8220154</a>
to the jdk/sandbox(<a moz-do-not-send="true" href="http://hg.openjdk.java.net/jdk/sandbox" class="">http://hg.openjdk.java.net/jdk/sandbox</a>).
<div class="">Corresponding webrev for the change is <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ejdv/8225160/webrev.00/" class="">http://cr.openjdk.java.net/~jdv/8225160/webrev.00/</a> .
This webrev has JetBrains code from <a moz-do-not-send="true" href="https://bugs.openjdk.java.net/browse/JDK-8220154" title="Improve java2d rendering performance on macOS by using
Metal framework" class="issue-link" data-issue-key="JDK-8220154">JDK-8220154</a> and our delta
modification[1] over jdk/sandbox metal-prototype-branch<br class="">
<br class="">
For additional info :
<br class="">
1. A webrev relative to the jdk/client codebase - this has
JetBrains webrev for <a moz-do-not-send="true" href="https://bugs.openjdk.java.net/browse/JDK-8220154" title="Improve java2d rendering performance on macOS by using
Metal framework" class="issue-link" data-issue-key="JDK-8220154">JDK-8220154</a> and our delta
modification
<br class="">
<a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Eaghaisas/8225160/JB_plus_delta/webrev.1/" class="">http://cr.openjdk.java.net/~aghaisas/8225160/JB_plus_delta/webrev.1/</a>
<br class="">
<br class="">
2. A webrev showing our delta modifications over JetBrains
webrev for <a moz-do-not-send="true" href="https://bugs.openjdk.java.net/browse/JDK-8220154" title="Improve java2d rendering performance on macOS by using
Metal framework" class="issue-link" data-issue-key="JDK-8220154">JDK-8220154</a>
<br class="">
<div class=""> <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Eaghaisas/8225160/webrev.1/" class="">http://cr.openjdk.java.net/~aghaisas/8225160/webrev.1/</a> </div>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
<div class="">[1] Delta modification: At native rendering side
Ajit has added delta modifications to implement FillRect and
DrawParallelogram logic using JetBrains initialisation logic.</div>
<div class=""><br class="">
</div>
<div class="">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.</div>
<div class="">Comparison of code at GraphicsConfig, SurfaceData
and Layer is captured under subtask : <a moz-do-not-send="true" href="https://bugs.openjdk.java.net/browse/JDK-8225165" class="">https://bugs.openjdk.java.net/browse/JDK-8225165</a> .</div>
<div class=""><br class="">
</div>
<div class="">Thanks,</div>
<div class="">Jay</div>
</div>
</blockquote>
</div>
</div></blockquote></div><br class=""></div></body></html>