Hi David, Thanks for taking a look!
> On 26 Mar 2018, at 01:03, David Holmes <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/ <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.02/> Updated webrev (incremental): http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01-02/ <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> wrote: >>> >>>> On Mar 22, 2018, at 10:34 AM, Robin Westberg <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. >>>
