Looks good to me. /Magnus
> 22 juni 2020 kl. 12:39 skrev Vladimir Kempik <vkem...@azul.com>: > > Hello > Thanks for looking into this Magnus. > > Here is updated version of webrev - > http://cr.openjdk.java.net/~vkempik/8243470/webrev.01/ > effectively it does exactly the same as previous version, I have removed the > version check for clang. > > > Thanks, Vladimir > >> 4 июня 2020 г., в 16:04, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> >> написал(а): >> >>> On 2020-06-03 20:07, Magnus Ihse Bursie wrote: >>> Thanks for cc:ing us. >>> >>> This is not the correct way to check for compiler versions. Nor is it the >>> correct place. >>> >>> I don't have time for a full reply tonight, but will return with a better >>> reply tomorrow. >> Ok, I have now looked more into this. >> >> You are apparently want to check for Xcode version, not clang version. This >> is not the same. Note that clang can be used on Linux as well. Apple is >> basically dropping the normal clang versioning and replacing it with their >> own, messed-up version which somewhat resembles the corresponding Xcode >> version, but is not identical. Horray. :-/ (For those interested, have a >> look at https://gist.github.com/yamaya/2924292.) >> >> Anyway. We do not support Xcode prior to 8. You seem to have tested from 8 >> and up, so this patch should be applied unconditionally. That is, just >> remove the -O1 special thing. >> >> /Magnus >>> >>> /Magnus >>> >>>> On 2020-06-03 19:47, Daniel D. Daugherty wrote: >>>> Adding build-dev@... since this is a build system change. >>>> >>>> As for the right HotSpot team, I'm not sure who should be making >>>> the call on changing the way unsafe.cpp is built for macOSX. My guess >>>> when I moved the bug to hotspot/runtime was that someone on the Runtime >>>> team would know, but... >>>> >>>> Dan >>>> >>>> >>>> >>>>> On 6/3/20 1:38 PM, Vladimir Kempik wrote: >>>>> Hello >>>>> >>>>> Can somebody please review this simple change ? >>>>> >>>>> Thanks >>>>> >>>>>> 6 мая 2020 г., в 12:43, Vladimir Kempik <vkem...@azul.com> написал(а): >>>>>> >>>>>> Adding hotspot-runtime-dev >>>>>> >>>>>>> 23 апр. 2020 г., в 18:26, Vladimir Kempik <vkem...@azul.com> написал(а): >>>>>>> >>>>>>> >>>>>>> Hello >>>>>>> Please review a fix for JDK-8243470 >>>>>>> >>>>>>> Long time ago as part of JEP284: New HotSpot Build System this fix was >>>>>>> applied to jdk9 https://bugs.openjdk.java.net/browse/JDK-8152666 >>>>>>> At that time it was decided to lower optimisation level for unsafe.cpp >>>>>>> from O2 to O1 only for clang on Macosx. >>>>>>> >>>>>>> I suppose it was done due to issues in Set/Get<Object> helper functions >>>>>>> where too optimistic optimisations were eliminating some null pointer >>>>>>> checks. it was probably a clang bug. >>>>>>> That issue could be checked with test jdk/test/sun/misc/CopyMemory.java. >>>>>>> >>>>>>> I believe that workaround (going from O2 to O1) produced this issue - >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8234963 (Thread.getStackTrace >>>>>>> is slow with clang). >>>>>>> JDK-8234963 can only be seen on mac with libjvm compiled by clang. >>>>>>> >>>>>>> Here I propose the patch which eliminates that workaround for clang 8+. >>>>>>> >>>>>>> I have tested clang versions 8/9/9.1/10, all of them showed good >>>>>>> results: >>>>>>> 1) CopyMemory test passes fine on 11/14/15. >>>>>>> 2) jdk11/jdk14 passed tck. Regression testing were good as well. jdk15: >>>>>>> no new failures in tck. >>>>>>> 3) The testRandom "benchmark" from 8234963 shows great improvements on >>>>>>> my machine, going down from ~200 seconds to ~150 seconds (the newer >>>>>>> clang the better result). For comparision, gcc built libjvm for jdk11 >>>>>>> shows ~130 seconds on my machine. >>>>>>> >>>>>>> The webrev: http://cr.openjdk.java.net/~vkempik/8243470/webrev.00/ >>>>>>> >>>>>>> getStackTrace benchmark: >>>>>>> http://cr.openjdk.java.net/~vkempik/8243470/TestRandom.java >>>>>>> >>>>>>> The bug: https://bugs.openjdk.java.net/browse/JDK-8243470 >>>>>>> >>>>>>> Thanks, Vladimir >>>> >>> >
