8302659: Modernize Windows native code for NetworkInterface #12593

rdicroce · 2023-02-16T14:32:15Z

Improves performance and correctness, as discussed on the net-dev mailing list.

Progress

Change requires CSR request JDK-8303477 to be approved
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8302659: Modernize Windows native code for NetworkInterface
JDK-8303477: Modernize Windows native code for NetworkInterface (CSR)

Reviewers

Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to 311cdc22
Daniel Jeliński (@djelinski - Committer)
Alan Bateman (@AlanBateman - Reviewer)
Michael McMahon (@Michael-Mc-Mahon - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12593/head:pull/12593
$ git checkout pull/12593

Update a local copy of the PR:
$ git checkout pull/12593
$ git pull https://git.openjdk.org/jdk pull/12593/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12593

View PR using the GUI difftool:
$ git pr show -t 12593

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12593.diff

…ce and correctness

bridgekeeper · 2023-02-16T14:33:07Z

👋 Welcome back rdicroce! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-02-16T14:35:57Z

@rdicroce The following labels will be automatically applied to this pull request:

build
core-libs
net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

dfuch · 2023-02-16T14:41:18Z

FWIW - there is a perl script located in make/scripts/normalizer.pl that can be run on modified sources to fix up whitespace and CRLF issues when jcheck complains.

mlbridge · 2023-02-16T14:45:12Z

Webrevs

AlanBateman · 2023-02-16T14:46:24Z

/label remove core-libs

openjdk · 2023-02-16T14:46:59Z

@AlanBateman
The core-libs label was successfully removed.

AlanBateman · 2023-02-16T14:49:30Z

Would it be possible to re-format the changes to src/java.base/windows/native/libnet/NetworkInterface.c to avoid the overly long lines - some of the new lines are 150-200 characters long so it will make impossible to look at side-by-side diffs in the future.

rdicroce · 2023-02-16T15:14:23Z

Sure. What do you want the max line length to be? 80 chars? 100?

dfuch · 2023-02-16T15:47:16Z

Something between 80 and 100 is usually a good limit. We typically avoid to go above that. So no hard limit at 80 but avoid having lines which are too long. If it displays correctly without wrapping using side diff in webrev / github side diff file view on a regular (for instance, laptop) screen it's a good sign.

magicus

Build changes look fine.

/reviewers 2

openjdk · 2023-02-16T16:33:10Z

@rdicroce This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8302659: Modernize Windows native code for NetworkInterface

Reviewed-by: ihse, djelinski, alanb, michaelm

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 505 new commits pushed to the master branch:

01e6920: 8298935: fix independence bug in create_pack logic in SuperWord::find_adjacent_refs
3d77e21: 8301308: Remove version conditionalization for gcc/clang PRAGMA_DIAG_PUSH/POP
e3777b0: 8270865: Print process ID with -Xlog:os
349139b: 8304030: Configure call fails on AIX when using --with-gtest option.
714b5f0: 8294962: Convert java.base/jdk.internal.module package to use the Classfile API to modify and write module-info.class
065d3e0: 8304171: Fix layout of JCov instrumented bundle on Mac OS
cd41c69: 8303705: Field sleeper.started should be volatile JdbLockTestTarg.java
f5c8b68: 8301998: Update HarfBuzz to 7.0.1
617c15f: 8304172: ProblemList serviceability/sa/UniqueVtableTest.java
f81e1de: 8303882: Refactor some iterators in jdk.compiler
... and 495 more: https://git.openjdk.org/jdk/compare/ecf21a9a24d067725fda916ab197b5711c56a1d7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@magicus, @djelinski, @AlanBateman, @Michael-Mc-Mahon) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2023-02-16T16:33:39Z

@magicus
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

rdicroce · 2023-02-16T16:44:16Z

Maximum line length reduced to 80-ish characters.

djelinski

Overall the changes look good. A few comments below.

src/java.base/windows/native/libnet/NetworkInterface.c

msheppar · 2023-02-16T18:01:43Z

what versions of Windows has this been tested on ? Just wondering if the deletion of the old dual stack winXP based code could have on the older windows OS platforms?
afaik current set of supported windows OS platforms, from the ATR list, are :
windows server 2019,
windows server 2022,
windows server 2016,
windows 10,
windows 11,
windows 8,
windows server2012,
windows server 2012R2

some of these may be retired for LTS JDK21, but for the moment they are in the mix.
We'll need to schedule test runs for all these.

Also we will have to arrange some IPv6 only test runs for this prior to any integration -- that takes a bit of time to arrange the resources to be setup and to schedule the runs.

rdicroce · 2023-02-16T18:08:36Z

@msheppar I tested this on Windows 10 and 11. I don't have access to any older systems. But I tried to be careful about which APIs I used. I'm pretty sure I checked the MS docs for all of them, and they all exist since Vista/Server 2008. So this should work on all the versions you listed.

msheppar · 2023-02-17T10:37:30Z

I don't think the NetworkInterface.h file should be dropped.
It is preferrable that typedef and struct decalarations remain in a header file rather than being merged into .c file.

src/java.base/windows/native/libnet/NetworkInterface.c

AlanBateman · 2023-02-28T19:10:33Z

Overall I think this is a very contribution. It's good to modernize this code and remove the cruft that dates from before many of the new APIs were added.

I suspect we will need a release document any behaviour changes that may be observed when upgrading the JDK. In particular, the network interface names will look different. The synthesized names were names like "lo" and "eth0" whereas the new names come from Windows (names like "lookup_0" and "ethernet_0"). I haven't found tools that show the names that ConvertInterfaceLuidToNameW returns but if there are then the RN could mention it. On other platforms, it would be was easy for configuration to name the interface to be used for multicasting, it doesn't seem easy to do the same on Windows and application might have to resort to configuration by IP address.

I'm not too concerned that interfaces without IP addresses are enumerated. They aren't useful for multicasting applications but maybe they are useful for informational purposes, e.g. programs that collect system environment. I'm also not too concern about the ordering of the IP addresses. I'll defer to @djelinski on whether some JDK tests or test infra needs to be updated in advance to ensure that tests that choose interfaces will continue to work.

djelinski · 2023-03-01T12:05:09Z

Ok then, let's keep the filter interfaces as they are; we can always revisit the issue later if there's demand.
I'm already working on updating the JCK tests.

rdicroce · 2023-03-01T14:37:34Z

I've updated the PR to address all of the various comments.

@AlanBateman Regarding tools that return the new interface names, I'm not aware of any either. But it's not too hard to hack together a PowerShell script to call if_indextoname which does exactly what the name implies.

djelinski

Network interface changes LGTM now. I'm not sure about the change to ResolverConfiguration, see below.

src/java.base/windows/native/libnet/ResolverConfigurationImpl.c

djelinski · 2023-03-01T18:57:17Z

Thanks for that!
I'm still running one last round of tests and will approve this if there are no new failures.

I'll start working on a CSR and a release note.

/csr needed

openjdk · 2023-03-01T18:58:56Z

@djelinski only the pull request author and Reviewers are allowed to use the csr command.

djelinski · 2023-03-14T10:10:43Z

CSR approved, release note ready. I think we're good to go. @rdicroce you can integrate the changes now.

rdicroce · 2023-03-14T12:00:51Z

Thanks @djelinski!

/integrate

openjdk · 2023-03-14T12:02:58Z

@rdicroce
Your change (at version 6a58575) is now ready to be sponsored by a Committer.

Michael-Mc-Mahon · 2023-03-15T16:32:58Z

I think the change looks good. We might need to put some more work into examining all details of behavior changes. For example, I noticed that previously the InetAddresses for each interface are returned IPv4 first, whereas now they are returned IPv6 first (on Windows 10 at least).

djelinski · 2023-03-15T17:05:07Z

/sponsor

openjdk · 2023-03-15T17:06:45Z

Going to push as commit 35a2969.
Since your change was applied there have been 505 commits pushed to the master branch:

01e6920: 8298935: fix independence bug in create_pack logic in SuperWord::find_adjacent_refs
3d77e21: 8301308: Remove version conditionalization for gcc/clang PRAGMA_DIAG_PUSH/POP
e3777b0: 8270865: Print process ID with -Xlog:os
349139b: 8304030: Configure call fails on AIX when using --with-gtest option.
714b5f0: 8294962: Convert java.base/jdk.internal.module package to use the Classfile API to modify and write module-info.class
065d3e0: 8304171: Fix layout of JCov instrumented bundle on Mac OS
cd41c69: 8303705: Field sleeper.started should be volatile JdbLockTestTarg.java
f5c8b68: 8301998: Update HarfBuzz to 7.0.1
617c15f: 8304172: ProblemList serviceability/sa/UniqueVtableTest.java
f81e1de: 8303882: Refactor some iterators in jdk.compiler
... and 495 more: https://git.openjdk.org/jdk/compare/ecf21a9a24d067725fda916ab197b5711c56a1d7...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-15T17:07:00Z

@djelinski @rdicroce Pushed as commit 35a2969.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

rdicroce added 2 commits February 15, 2023 13:56

Rewrite Windows native code for NetworkInterface to improve performan…

8385c4c

…ce and correctness

Remove executable bit from NetworkInterface.c

7bcdcd8

openjdk bot added build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org labels Feb 16, 2023

Fix whitespace issues

311cdc2

openjdk bot added the rfr Pull request is ready for review label Feb 16, 2023

openjdk bot removed the core-libs core-libs-dev@openjdk.org label Feb 16, 2023

magicus approved these changes Feb 16, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 16, 2023

openjdk bot removed the ready Pull request is ready to be integrated label Feb 16, 2023

Limit line length to 80-ish characters

b23d139

djelinski reviewed Feb 16, 2023

View reviewed changes

rdicroce added 2 commits February 22, 2023 12:27

Resolve review comments

68b35cc

Forgot to add file

da66b50

AlanBateman reviewed Feb 27, 2023

View reviewed changes

src/java.base/windows/native/libnet/NetworkInterface.c Show resolved Hide resolved

AlanBateman reviewed Feb 28, 2023

View reviewed changes

src/java.base/windows/native/libnet/NetworkInterface.c Show resolved Hide resolved

AlanBateman reviewed Feb 28, 2023

View reviewed changes

src/java.base/windows/native/libnet/NetworkInterface.c Outdated Show resolved Hide resolved

Changes as per review comments

f2ce5aa

djelinski reviewed Mar 1, 2023

View reviewed changes

src/java.base/windows/native/libnet/ResolverConfigurationImpl.c Outdated Show resolved Hide resolved

Revert flag change in ResolverConfigurationImpl

6a58575

openjdk bot added the csr Pull request needs approved CSR before integration label Mar 1, 2023

djelinski approved these changes Mar 2, 2023

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Mar 13, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 14, 2023

AlanBateman approved these changes Mar 14, 2023

View reviewed changes

Michael-Mc-Mahon approved these changes Mar 15, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 15, 2023

openjdk bot closed this Mar 15, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 15, 2023

8302659: Modernize Windows native code for NetworkInterface #12593

8302659: Modernize Windows native code for NetworkInterface #12593

rdicroce commented Feb 16, 2023 •

edited by openjdk bot

bridgekeeper bot commented Feb 16, 2023

openjdk bot commented Feb 16, 2023

dfuch commented Feb 16, 2023

mlbridge bot commented Feb 16, 2023 •

edited

AlanBateman commented Feb 16, 2023

openjdk bot commented Feb 16, 2023

AlanBateman commented Feb 16, 2023

rdicroce commented Feb 16, 2023

dfuch commented Feb 16, 2023 •

edited

magicus left a comment

openjdk bot commented Feb 16, 2023 •

edited

openjdk bot commented Feb 16, 2023

rdicroce commented Feb 16, 2023

djelinski left a comment

msheppar commented Feb 16, 2023

rdicroce commented Feb 16, 2023

msheppar commented Feb 17, 2023

AlanBateman commented Feb 28, 2023

djelinski commented Mar 1, 2023

rdicroce commented Mar 1, 2023

djelinski left a comment

djelinski commented Mar 1, 2023

openjdk bot commented Mar 1, 2023

djelinski commented Mar 14, 2023

rdicroce commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

Michael-Mc-Mahon commented Mar 15, 2023

djelinski commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

8302659: Modernize Windows native code for NetworkInterface #12593

8302659: Modernize Windows native code for NetworkInterface #12593

Conversation

rdicroce commented Feb 16, 2023 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Feb 16, 2023

openjdk bot commented Feb 16, 2023

dfuch commented Feb 16, 2023

mlbridge bot commented Feb 16, 2023 • edited

Webrevs

AlanBateman commented Feb 16, 2023

openjdk bot commented Feb 16, 2023

AlanBateman commented Feb 16, 2023

rdicroce commented Feb 16, 2023

dfuch commented Feb 16, 2023 • edited

magicus left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 16, 2023 • edited

openjdk bot commented Feb 16, 2023

rdicroce commented Feb 16, 2023

djelinski left a comment

Choose a reason for hiding this comment

msheppar commented Feb 16, 2023

rdicroce commented Feb 16, 2023

msheppar commented Feb 17, 2023

AlanBateman commented Feb 28, 2023

djelinski commented Mar 1, 2023

rdicroce commented Mar 1, 2023

djelinski left a comment

Choose a reason for hiding this comment

djelinski commented Mar 1, 2023

openjdk bot commented Mar 1, 2023

djelinski commented Mar 14, 2023

rdicroce commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

Michael-Mc-Mahon commented Mar 15, 2023

djelinski commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

rdicroce commented Feb 16, 2023 •

edited by openjdk bot

mlbridge bot commented Feb 16, 2023 •

edited

dfuch commented Feb 16, 2023 •

edited

openjdk bot commented Feb 16, 2023 •

edited