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.usha...@jetbrains.com <mailto:alexey.usha...@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....@oracle.com
<mailto:jayathirth....@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.r...@oracle.com
<mailto:philip.r...@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