+1. Thanks, Jay
> On 12-Jun-2020, at 12:16 AM, Philip Race <philip.r...@oracle.com> wrote: > > > one more update to the tests > cr.openjdk.java.net/~prr/8240654.2/ > > I added > @requires vm.gc.Z > > per the VM folks, ZGC needs Windows Server 2019 or the same vintage Window > 10. if run on Windows Server 2016 > ZGC errors out. This should prevent that. > > -phil. > > On 6/11/2020 11:13 AM, Kevin Rushforth wrote: >> +1 >> >> The updated LargeWindowPaintTest fails for me without the fix without my >> having to manually set uiScale. It passes with the fix. >> >> Interestingly enough, I finally saw the problem that Jay reported with >> AlphaPrintTest: without the fix I initially get a blank (all white) window. >> If I resize it then it is drawn. With the fix everything is fine. >> >> -- Kevin >> >> On 6/11/2020 10:48 AM, Philip Race wrote: >>> Updated webrev here >>> >>> http://cr.openjdk.java.net/~prr/8240654.1/index.html >>> <http://cr.openjdk.java.net/~prr/8240654.1/index.html> >>> >>> The only changes are to the tests - to add -Dsun.java2d.uiScale=1 to the >>> onscreen test >>> and to add printer to the keys for the printing test. >>> >>> It was pointed out by Stefan from the ZGC team that the changes in >>> awt_TrayIcon.cpp and awt_Cursor.cpp >>> should not be needed because the GDI code in Create_BMP that ultimately >>> consumes the data >>> has processed and copied it into memory allocated by CreateDIBSection >>> before passing it to >>> CreateBitmap. I considered reverting those two files but decided to keep >>> them because I >>> think I would like this fix anyway. We really don't need to lock down the >>> VM in these cases. >>> >>> -phil. >>> >>> >>> On 6/11/20, 9:55 AM, Philip Race wrote: >>>> >>>> I have confirmed hit a different code path. It goes through generic 2D s/w >>>> loops in this case. >>>> ie we don't use GDIBlitLoops at all. The code in >>>> sun/java2d/pipe/DrawImage.java ends up in >>>> scaleSurfaceData which uses the loops in ScaledBlit.c. >>>> >>>> It is a bit surprising to me since I'd expect us to be able to blit >>>> directly at device resolution. >>>> Could we also be taking a performance hit here ? The D3D case doesn't not >>>> go through this loop. >>>> >>>> However all that is outside the scope of this fix ... I think setting >>>> uiScale=1 in the test is all that needs to be done. >>>> >>>> -phil. >>>> >>>> On 6/11/2020 7:51 AM, Philip Race wrote: >>>>> Or, maybe we hit a different code path. I'll check that. >>>>> uiScale=1 is the way to ensure we hit this code path. >>>>> >>>>> -phil. >>>>> >>>>> On 6/11/20, 7:44 AM, Philip Race wrote: >>>>>> >>>>>> Can I get clarification here. >>>>>> >>>>>> > I do, and had to run with "-Dsun.java2d.uiScale=1" in order to see the >>>>>> > failure with LargeWindowPaintTest. >>>>>> >>>>>> So you both mean a JDK 15 promoted build without this fix and without >>>>>> this property passes because you have >>>>>> a hidpi setup. And to see the failure without the fix you needed the >>>>>> above property. >>>>>> If so we could just be looking at a similar anomaly as I saw with >>>>>> printing which uses a very large >>>>>> image - it reported failure but actually worked ! >>>>>> >>>>>> Also - for both of you - with the fix and without forcing uiScale=1 does >>>>>> the test pass ? >>>>>> >>>>>> -phil. >>>>>> >>>>>> On 6/11/20, 7:10 AM, Jayathirth D v wrote: >>>>>>> >>>>>>> Yes my machine was at 150% scaling. >>>>>>> >>>>>>> If I force uiScale = 1, I see that: >>>>>>> LargeWindowPaintTest fails without patch and passes with patch. >>>>>>> AlphaPrintTest shows instructions without patch also. >>>>>>> >>>>>>> @Phil : I think its better if we test at uiScale=1(larger memory >>>>>>> footprint). Please clarify. >>>>>>> >>>>>>> Thanks, >>>>>>> Jay >>>>>>> >>>>>>>> On 11-Jun-2020, at 5:53 PM, Kevin Rushforth >>>>>>>> <kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com>> wrote: >>>>>>>> >>>>>>>> Do you have a Hi-DPI machine? I do, and had to run with >>>>>>>> "-Dsun.java2d.uiScale=1" in order to see the failure with >>>>>>>> LargeWindowPaintTest. >>>>>>>> >>>>>>>> For AlphaPrintTest, the test deliberately ensures that you print >>>>>>>> before saying whether it passes or not. FWIW, I verified that the >>>>>>>> printing test on my system was hitting the fallback code with the >>>>>>>> patch, but it seemed to print correctly even without the patch. >>>>>>>> >>>>>>>> -- Kevin >>>>>>>> >>>>>>>> >>>>>>>> On 6/11/2020 1:58 AM, Jayathirth D v wrote: >>>>>>>>> Typo : I tried tested -> I tried testing >>>>>>>>> >>>>>>>>>> On 11-Jun-2020, at 2:27 PM, Jayathirth D v >>>>>>>>>> <jayathirth....@oracle.com <mailto:jayathirth....@oracle.com>> wrote: >>>>>>>>>> >>>>>>>>>> Hi Phil, >>>>>>>>>> >>>>>>>>>> I tried tested the fix in my Windows 10 machine with Intel >>>>>>>>>> integrated UHD Graphics 620. >>>>>>>>>> >>>>>>>>>> LargeWindowPaintTest.java passes with/without fix in my machine. >>>>>>>>>> AlphaPrintTest.java without fix just opens up blank frame without >>>>>>>>>> any instructions and with fix it shows instructions for the test. >>>>>>>>>> Is this expected behaviour? >>>>>>>>>> >>>>>>>>>> AlphaPrintTest.java with fix when it shows instructions if I click >>>>>>>>>> on Pass(Since I don’t have printer right now) it doesn’t pass/close >>>>>>>>>> the window. Only after I click on Print button and then close print >>>>>>>>>> dialog it allows me to click on Pass button. >>>>>>>>>> >>>>>>>>>> Also how does these tests behave in our internal CI machines? >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Jay >>>>>>>>>> >>>>>>>>>>> On 11-Jun-2020, at 2:18 AM, Philip Race <philip.r...@oracle.com >>>>>>>>>>> <mailto:philip.r...@oracle.com>> wrote: >>>>>>>>>>> >>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8240654 >>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8240654> >>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~prr/8240654/index.html >>>>>>>>>>> <http://cr.openjdk.java.net/%7Eprr/8240654/index.html> >>>>>>>>>>> >>>>>>>>>>> This is for JDK 15 so review ASAP please since RDP 1 and the test >>>>>>>>>>> cycle are looming. >>>>>>>>>>> >>>>>>>>>>> This is not a fix for a JDK bug. It is a bunch of workarounds for a >>>>>>>>>>> Microsoft Windows bug affecting >>>>>>>>>>> GDI in the context of ZGC (http://openjdk.java.net/jeps/333 >>>>>>>>>>> <http://openjdk.java.net/jeps/333>). >>>>>>>>>>> Some extra details about the Windows bug at the end, but first the >>>>>>>>>>> technical details of the fix. >>>>>>>>>>> >>>>>>>>>>> With ZGC's memory allocation requirement of reserving memory in 2Mb >>>>>>>>>>> chunks some Windows GDI >>>>>>>>>>> functions, mostly involving some bitmaps APIs may return a failure >>>>>>>>>>> code (ie fail!) >>>>>>>>>>> This typically occurs when Java heap memory is used for a Java >>>>>>>>>>> image and then in a JNI >>>>>>>>>>> call we use GetPrimitiveArrayCritical so that Java heap allocated >>>>>>>>>>> memory is passed to a GDI >>>>>>>>>>> function AND the Java heap memory spans one of the 2Mb boundaries. >>>>>>>>>>> This is very easy to trigger in almost any Java UI app if the >>>>>>>>>>> window is of a large enough (ie typical) size. >>>>>>>>>>> NB: if you have an Nvidia or ATI card, then you won't see it, >>>>>>>>>>> because the D3D pipeline doesn't >>>>>>>>>>> call the affected method but if you have an Intel chip as do 90% >>>>>>>>>>> (?) of laptops you will see it. >>>>>>>>>>> There are also several other places we found that are affected. >>>>>>>>>>> Printing is the other one >>>>>>>>>>> somewhat easy to trigger. The others : custom cursors and tray >>>>>>>>>>> icons are less common. >>>>>>>>>>> The painful thing here is that there is no definitive list (a list >>>>>>>>>>> of the known ones is below) of >>>>>>>>>>> affected Windows GDI APIs and we are just hunting around our code >>>>>>>>>>> trying to see where it >>>>>>>>>>> might be side-swiped by this bug. >>>>>>>>>>> >>>>>>>>>>> The basic approach in these workarounds is that for cases where >>>>>>>>>>> performance does not matter we now copy >>>>>>>>>>> and for cases where performance does matter or larger amounts of >>>>>>>>>>> memory is involved we check if >>>>>>>>>>> the return value of the GDI function indicates failure and then >>>>>>>>>>> re-try with a copy of the heap memory. >>>>>>>>>>> Unless GDI was randomly failing already (unlikely) this should be a >>>>>>>>>>> no-risk solution in the high profile cases. >>>>>>>>>>> We have done performance measurements on the important screen case >>>>>>>>>>> and the failures >>>>>>>>>>> happen fast so the penalty is then in the re-try which is only if >>>>>>>>>>> ZGC is enabled. >>>>>>>>>>> Always copying the memory is slower (and memcpy is the slow >>>>>>>>>>> operation) than an alternative approach >>>>>>>>>>> that "knows" about the memory allocation of ZGC but this coupling >>>>>>>>>>> and the complexity seem like they aren't >>>>>>>>>>> worth it since I haven't seen any visible performance consequence. >>>>>>>>>>> That can be revisited >>>>>>>>>>> some day if need be, but for now we have correctness which is the >>>>>>>>>>> key as well as sufficient performance. >>>>>>>>>>> >>>>>>>>>>> I've created an automated test for the most important on-screen >>>>>>>>>>> case. >>>>>>>>>>> Also a manual printing test case which invokes ZGC is provided >>>>>>>>>>> since there we also only >>>>>>>>>>> conditionally copy. In the other cases we now always copy so >>>>>>>>>>> existing test cases should over those. >>>>>>>>>>> >>>>>>>>>>> There is some clean up in this fix - one completely unused >>>>>>>>>>> (provably so because it was #if'd out) >>>>>>>>>>> JNI method in awt_PrintJob.cpp is removed since it had code that >>>>>>>>>>> looked like it needed a workaround, >>>>>>>>>>> which would be somewhat of a waste of effort. >>>>>>>>>>> >>>>>>>>>>> the doPrintBand code and its callee bitsToDevice has code I think >>>>>>>>>>> we can remove too since >>>>>>>>>>> I don't see how it ever gets executed (the top down case for >>>>>>>>>>> browserPrint == true) but >>>>>>>>>>> I think I'll save that for a P4 follow-on since it does nothing >>>>>>>>>>> that would be affected by this >>>>>>>>>>> Windows bug. >>>>>>>>>>> >>>>>>>>>>> One oddity is the in the printing case I observed that some times >>>>>>>>>>> the rendering is performed >>>>>>>>>>> even if an error code is returned. I don't know why, but in code we >>>>>>>>>>> can't tell that it was actually >>>>>>>>>>> rendered and in any case there is no harm in repeating the call >>>>>>>>>>> with copied memory. >>>>>>>>>>> >>>>>>>>>>> We are right before the JDK15 stabilisation fork and this fix needs >>>>>>>>>>> to go there and will >>>>>>>>>>> but the webrev is against jdk/client simply because jdk15 does not >>>>>>>>>>> exist yet ! >>>>>>>>>>> >>>>>>>>>>> Please test and review ASAP. >>>>>>>>>>> >>>>>>>>>>> About the bug: >>>>>>>>>>> Microsoft has acknowleged the bug and will publish a knowledge base >>>>>>>>>>> article about it >>>>>>>>>>> but a fix may show up only in a future version of Windows. Not, it >>>>>>>>>>> seems, any time soon. >>>>>>>>>>> Below is a list of potentially affected GDI APIs. Per microsoft >>>>>>>>>>> whether it actually manifests in >>>>>>>>>>> any specific case depends on "branching" >>>>>>>>>>> https://docs.microsoft.com/en-us/previous-versions/windows/desktop/wcs/checkbitmapbits >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/previous-versions/windows/desktop/wcs/checkbitmapbits> >>>>>>>>>>> https://docs.microsoft.com/en-us/previous-versions/windows/desktop/wcs/createcolortransform >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/previous-versions/windows/desktop/wcs/createcolortransform> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-setdibitstodevice >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-setdibitstodevice> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-stretchdibits >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-stretchdibits> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-getbitmapbits >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-getbitmapbits> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-createdibitmap >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-createdibitmap> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-createdibsection >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-createdibsection> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-polydraw >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-polydraw> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-drawescape >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-drawescape> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-createbitmap >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-createbitmap> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-setbitmapbits >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-setbitmapbits> >>>>>>>>>>> https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-getdibits >>>>>>>>>>> >>>>>>>>>>> <https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-getdibits> >>>>>>>>>>> >>>>>>>>>>> -phil. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>> >> >