Looks good to me.
Thanks,
David
On 27/03/2018 1:01 AM, Robin Westberg wrote:
Hi David,
Thanks for taking a look!
On 26 Mar 2018, at 01:03, David Holmes <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>> wrote:
Hi Robin,
On 23/03/2018 10:37 PM, Robin Westberg wrote:
Hi Kim & Erik,
Certainly makes sense to define it from the build system, I’ve
updated the patch accordingly:
Full: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/
<http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/>
Incremental:
http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/
<http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/>
I'm a little unclear on the hotspot changes. If we define
WIN32_LEAN_AND_MEAN then certain APIs like sockets are excluded from
windows.h so we then have to include the specific header files like
winsock2.h - is that right?
Yep that’s correct, headers like winsock, dde, ole, shellapi and a few
other uncommon ones are no longer included from windows.h when this is
defined.
src/hotspot/share/interpreter/bytecodes.cpp
I'm curious about this change. u_short comes from types.h on
non-Windows, is it simply missing on Windows (at least once we have
WIN32_LEAN_AND_MEAN defined) ?
Yeah, on Windows these comes from winsock(2).h:
/*
* Basic system type definitions, taken from the BSD file sys/types.h.
*/
typedef unsigned char u_char;
typedef unsigned short u_short;
typedef unsigned int u_int;
typedef unsigned long u_long;
I noticed that one of these (u_char) is also defined in
globalDefinitions.hpp so could perhaps define u_short there, or include
winsock2.h globally again. But since it was only used in a single place
in the existing code it seemed simple enough to just expand the typedef
there.
src/hotspot/share/utilities/ostream.cpp
1029 #endif
1030 #if defined(_WINDOWS)
Using elif could be marginally faster given the two sets of conditions
are mutually exclusive.
Good point, will change it.
I also had to move the flag definition to adapt to the latest changes in
the hs repo, cc’ing build-dev again to make sure I got it right.
Updated webrev (full):
http://cr.openjdk.java.net/~rwestberg/8199736/webrev.02/
Updated webrev (incremental):
http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01-02/
Best regards,
Robin
Thanks,
David
(Not quite sure if the definition belongs where I put it or a bit
later where most other windows-specific JVM flags are defined, but
seemed reasonable to put it close to where it is defined for the JDK
libraries).
Best regards,
Robin
On 22 Mar 2018, at 16:52, Kim Barrett <kim.barr...@oracle.com
<mailto:kim.barr...@oracle.com>> wrote:
On Mar 22, 2018, at 10:34 AM, Robin Westberg
<robin.westb...@oracle.com <mailto:robin.westb...@oracle.com>> wrote:
Hi all,
Please review the following change that defines WIN32_LEAN_AND_MEAN
[1] before including windows.h. This marginally improves build
times, and makes it possible to include winsock2.h.
Issue: https://bugs.openjdk.java.net/browse/JDK-8199736
<https://bugs.openjdk.java.net/browse/JDK-8199736>
Webrev: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/
<http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/>
Testing: hs-tier1
Best regards,
Robin
[1]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx#faster_builds_with_smaller_header_files
<https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx#faster_builds_with_smaller_header_files>
I think the addition of the WIN32_LEAN_AND_MEAN definition should be
done through the build
system, so that it applies everywhere.
