RFR: 8248817: Windows: Improving common cross-platform code
David Holmes
david.holmes at oracle.com
Wed Jul 15 13:32:37 UTC 2020
Hi Ludovic,
On 15/07/2020 11:15 pm, Ludovic Henry wrote:
> Hi David,
>
> Thanks for your feedback.
>
>> can we use __int32 for clarity rather than "long"?
>
> The Win32 API explicitly uses `long`, and I made sure for these `DEFINE_` macros to use the type used in the declaration of the API. If you are ok with the difference, I'm happy to change that to __int32.
I prefer to see _int32 in our code as "long" can be quite ambiguous
depending on the reader (and something we are trying to eradicate from
shared code -not everyone is aware of the LLP64 programming model versus
LP64).
Thanks,
David
-----
> I've uploaded the new webrevs at http://cr.openjdk.java.net/~burban/luhenry/8248817-atomics/webrev.01
>
> (I've also moved the previous webrevs to their respective webrev.00 folders).
>
> Thank you,
>
> --
> Ludovic
> ________________________________________
> From: David Holmes <david.holmes at oracle.com>
> Sent: Sunday, July 12, 2020 19:43
> To: Ludovic Henry; hotspot-runtime-dev at openjdk.java.net
> Cc: openjdk-aarch64
> Subject: Re: RFR: 8248817: Windows: Improving common cross-platform code
>
> Hi Ludovic,
>
> On 9/07/2020 11:55 pm, Ludovic Henry wrote:
>> Hello,
>>
>> As part of adding support for Windows-AArch64, I've had the opportunity to read through most of the Windows-x86 code. In doing so, I found some code that I think can be simplified and made easier to read and maintain.
>>
>> The three areas I have found are:
>> - Atomics: Hotspot doesn't make use of existing intrinsics provided by MSVC and Win32, even ones available since Windows XP.
>> - Exception handling: there is some code repetition which, even if functional, is subpar.
>> - Frames: we can use the existing os::fetch_frame_from_context to simplify the code and reduce frame parsing logic duplication.
>>
>> I've split the webrevs along the above lines, making each simpler to review. I'm also hosting these webrevs on Bernhard Urban's CR as I currently do not have authorship. I'll also work with him to update the description of the JBS.
>
> Thanks for doing the split!
>
> As a general comment can you please ensure that the Oracle copyright
> second year is updated to 2020. Thanks.
>
> Overall these cleanups look good. Thanks for providing them.
>
>> JBS: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8248817&data=02%7C01%7Cluhenry%40microsoft.com%7C14256cbf115b41c4e39c08d826d68c6a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637302050209431654&sdata=V5uMNQ%2FLK6enIbupfof66gviFxjLRzVLlrNAwW0r0JU%3D&reserved=0
>> Webrevs:
>> https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~burban%2Fluhenry%2F8248817-atomics%2F&data=02%7C01%7Cluhenry%40microsoft.com%7C14256cbf115b41c4e39c08d826d68c6a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637302050209431654&sdata=%2B63SBk1XCKXuCPC1ciOI2J2lhZd%2FM7jRbzciiGvYBbE%3D&reserved=0
>
> Love this cleanup! Great to see all the stubroutines go for x86.
>
> src/hotspot/os_cpu/windows_x86/atomic_windows_x86.hpp
>
> Please delete this entire (archaic) comment block.
>
> 42 // The following alternative implementations are needed because
> 43 // Windows 95 doesn't support (some of) the corresponding Windows NT
> 44 // calls. Furthermore, these versions allow inlining in the caller.
> 45 // (More precisely: The documentation for InterlockedExchange says
> 46 // it is supported for Windows 95. However, when single-stepping
> 47 // through the assembly code we cannot step into the routine and
> 48 // when looking at the routine address we see only garbage code.
> 49 // Better safe then sorry!). Was bug 7/31/98 (gri).
> 50 //
> 51 // Performance note: On uniprocessors, the 'lock' prefixes are not
> 52 // necessary (and expensive). We should generate separate cases if
> 53 // this becomes a performance problem.
>
> In this (and elsewhere):
>
> 80 DEFINE_STUB_ADD(4, long, InterlockedAdd)
> 81 DEFINE_STUB_ADD(8, __int64, InterlockedAdd64)
>
> can we use __int32 for clarity rather than "long"?
>
>> https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~burban%2Fluhenry%2F8248817-exception-handling%2F&data=02%7C01%7Cluhenry%40microsoft.com%7C14256cbf115b41c4e39c08d826d68c6a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637302050209441605&sdata=VEk0oD7xvDH4vrU4TF2qsQpw%2B7XLANcEcc2K5f%2BOCeQ%3D&reserved=0
>
> Looks good!
>
>> https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~burban%2Fluhenry%2F8248817-frames%2F&data=02%7C01%7Cluhenry%40microsoft.com%7C14256cbf115b41c4e39c08d826d68c6a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637302050209441605&sdata=tWcWMTclQIGmqtcIJUKzU8ksNMsbr0HxQeJ%2BeepKVTU%3D&reserved=0
>
> Looks good!
>
> Thanks,
> David
> -----
>
>> Tests: jtreg:hotspot:tier, jtreg:jdk:tier1, jtreg:jdk:tier2, jtreg:langtools on Windows-x86 and Windows-x86_64, no regressions.
>>
>> Thank you,
>>
>> --
>> Ludovic
>>
More information about the hotspot-runtime-dev
mailing list