Thanks Daniel! I've removed the executable bit (my own fault for moving files around in Windows Explorer...) and opened a PR here:
https://github.com/openjdk/jdk/pull/12593 Rich DiCroce Senior Advanced Solutions Architect Scientific Games HAVE FUN. DO GOOD. PLAY HEALTHY. May be privileged. May be confidential. Please delete if not the addressee. -----Original Message----- From: net-dev <net-dev-r...@openjdk.org> On Behalf Of Daniel Jelinski Sent: Thursday, February 16, 2023 9:24 AM To: net-dev@openjdk.org Subject: Re: Performance of NetworkInterface methods on Windows WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe. Hi Rich, Thanks for working on this. Please create a PR and put "8302659" (without the quotes) in the PR title; this will link the PR to the JBS issue. You may also need to remove the executable bit from NetworkInterface.c to make the bots happy. We'll continue the review on the PR. I need more time to evaluate the issues you posted. I will reply later. Regards, DJ śr., 15 lut 2023 o 20:59 DiCroce, Richard <rich.dicr...@scientificgames.com> napisał(a): > > I've pushed a branch that rewrites the native code as per the last > email. You can find it here: > https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith > ub.com%2Frdicroce%2Fjdk%2Ftree%2Fwindows-ni-rewrite&data=05%7C01%7CRic > h.DiCroce%40scientificgames.com%7Ca8e3c5bd927f46174ece08db102997ea%7Cb > 9d2c79d8655430e8cbbd458178fd6b8%7C0%7C0%7C638121543067799444%7CUnknown > %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ > XVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FFFffAa%2BilqURkkp2dGyS6pg19e0Ehk35R > 7lF08acZw%3D&reserved=0 > > In the process of doing this work, I discovered a few things that are broken > in the existing code: > > 1) The calls to GetAdaptersAddresses do not include the > GAA_FLAG_INCLUDE_ALL_INTERFACES flag. That is a problem because filters (and > maybe other things, I didn't look too thoroughly and the docs aren't clear) > are not included in the result unless that flag is set. But when enumerating > interfaces, the XP code begins by calling the non-XP code, which DOES > enumerate all those filters. Therefore, if you call > isUp/getMTU/supportsMulticast/etc on any of those filters and the XP code is > in use, you get bogus results because the XP code can't find the interface. > > 2) The NoMulticast flag in GetAdaptersAddresses doesn't seem to be set under > any conditions, at least not on Windows 11. When I disabled multicast on a > Win 11 VM, attempting to join a multicast group failed, as it should. But > GetAdaptersAddresses still didn't set the NoMulticast flag. > > #1 is pretty bad and easily fixed, so it might be worth backporting a narrow > fix for JDK 11/17. > > Now, on to my proposed changes. Most of the new code is straightforward, > except for the new implementation of supportsMulticast. I left a giant > comment explaining all the things I tried that didn't work there. On my > machine, the new code produces the same results as the existing code, except > for the following: > > A) isP2P sometimes returns different results. But the new code leans on the > OS to determine the access type, instead of looking at the interface type and > trying to guess from there. So the new code should be more accurate. > Currently, the code only returns true if the access type is > NET_IF_ACCESS_POINT_TO_POINT. I'm not sure if it should also return true for > NET_IF_ACCESS_POINT_TO_MULTI_POINT. Thoughts? > > B) The interface name is different, as mentioned in the previous email. For > example, on my machine, "wlan0" is now "wireless_32769". Since nobody had any > opinions about backwards compatibility, I didn't add any. IMO this change > should just be mentioned in the release notes. > > C) On my machine, most methods are now several orders of magnitude faster. > One notable exception is supportsMulticast, which is several times slower. > But as explained in the comments, I don't see a better solution, other than > just saying "forget it" and always returning true because it's highly > unlikely that multicast is disabled. > > So please take a look at the changes and let me know what you think. > > Thanks, > > Rich DiCroce > Senior Advanced Solutions Architect > > Scientific Games > > > HAVE FUN. DO GOOD. PLAY HEALTHY. > May be privileged. May be confidential. Please delete if not the addressee. > > > -----Original Message----- > From: net-dev <net-dev-r...@openjdk.org> On Behalf Of DiCroce, Richard > Sent: Wednesday, February 8, 2023 11:27 AM > To: net-dev@openjdk.org > Subject: RE: Performance of NetworkInterface methods on Windows > > WARNING: This is an external email. Do not click links or open attachments > unless you recognize the sender and know the content is safe. > > > Okay, so based on the various replies I've gotten, it sounds like we want a > patch that: > > 1) Completely rewrites the native code using newer APIs, and gets rid > of the dual implementations that exist today (per Daniel Jelinski) > 2) Gets rid of the homebrew interface naming scheme in favor of what > the Windows API returns (per Alan Bateman) > > Do we care about backwards compatibility for NetworkInterface#getByIndex() > and getByName()? Based on my experiments, I don't think the indexes will > actually change, but I can't 100% guarantee that. The names will definitely > change though, per #2 above. How do we want to handle that? Do we just put > something in the release notes saying there's a change that's not > backwards-compatible? Or do we want to do a fallback to the existing naming > scheme if the Windows API doesn't recognize a name? Or something else > entirely, like a system property to toggle the behavior? > > > Rich DiCroce > Senior Advanced Solutions Architect > > Scientific Games > > > HAVE FUN. DO GOOD. PLAY HEALTHY. > May be privileged. May be confidential. Please delete if not the addressee. > > > -----Original Message----- > From: Daniel Fuchs <daniel.fu...@oracle.com> > Sent: Wednesday, February 8, 2023 7:44 AM > To: DiCroce, Richard <rich.dicr...@scientificgames.com>; > net-dev@openjdk.org > Subject: Re: Performance of NetworkInterface methods on Windows > > WARNING: This is an external email. Do not click links or open attachments > unless you recognize the sender and know the content is safe. > > > Hi Richard, > > On 07/02/2023 21:04, DiCroce, Richard wrote: > > Hi Daniel, > > I'm not proposing to add any caching, or change anything about the Java > > side of NetworkInterface, for exactly the reasons you state. This first > > patch is purely a change to the native code to make it so that getAll only > > calls the Windows API once instead of N + 1 times. > > Oh, OK - good to hear. That sounds interesting then, sorry for assuming! > :-) > > > createNetworkInterface actually already had parameters for passing the > > previously fetched list of interfaces. getAll just doesn't currently do > > that. > > > > This would probably be easier to discuss if you could actually see the > > changes I'm proposing. Should I attach a patch to an email, or is there a > > better way of doing that? > > Jaikiran replied with a good suggestion for that. But please take into > account Daniel Jelinski's comments first. > > best regards, > > -- daniel > > > > > Thanks, > > > > Rich DiCroce >
