Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update NUMA_NODE_RELATIONSHIP to new variable size structure #1363

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

dbwiddis
Copy link
Contributor

@dbwiddis dbwiddis commented Aug 8, 2021

See background in #1324.

API Reference: https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-numa_node_relationship

Mapping directly to the API union loses backwards compatibility. This version populates the variable sized array of the newer union, and copies that value to a backwards-compatible field that is accessible to users but excluded from the Structure's field lists.

@dbwiddis dbwiddis force-pushed the numa branch 4 times, most recently from cc3a87a to 901e443 Compare August 8, 2021 21:54
@dbwiddis dbwiddis marked this pull request as ready for review August 8, 2021 21:55
@dbwiddis dbwiddis force-pushed the numa branch 3 times, most recently from 12dc9a3 to bf297aa Compare August 8, 2021 22:07
Comment on lines 3189 to 3191
if (groupCount > 1) {
groupMasks = new GROUP_AFFINITY[groupCount];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to replace with:

Suggested change
if (groupCount > 1) {
groupMasks = new GROUP_AFFINITY[groupCount];
}
groupMasks = new GROUP_AFFINITY[Math.max(groupCount, 1)];

that way multiple reads will work, even if the structure is wrapped once over old style Structure and once over new style structure.

Copy link
Contributor Author

@dbwiddis dbwiddis Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether it's "old" (0) or "new" (>0) is an OS-level condition that won't change during execution. But conceivably if someone wanted to re-use a structure and just change the native bits with useMemory() the value could change. But in this case, the proposed change would recreate the array (and reinitializing/recreating its internal Structure objects and the ULONG_PTR) on every read(). What we really want is to conditionally resize the array only if it's a new version and has changed.

Plus, in the vast majority of cases (systems with <= 64 processors = 1 processor group) the groupCount is 1 and the array is already the correct size.

So I'd propose:

if (groupCount > 0 && groupCount != groupMasks.length) {
    groupMasks = new GROUP_AFFINITY[groupCount];
}

Copy link
Contributor Author

@dbwiddis dbwiddis Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, while I think it would be highly unusual for this value to change on re-read (requires both changing the number of processor groups while the program was operating (e.g., using a hypervisor and changing from under to over 64 logical processors) and re-using an existing structure rather than generating a new one) we do something similar to your proposal in the PROCESSOR_RELATIONSHIP structure, so it's a reasonable approach from the standpoint of consistency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you really want to optimize, you could do this (there needs to be at least one entry in the array (the original single value), reinitialization of the array is only needed if size changes:

        @Override
        public void read() {
            // resize array if needed
            readField("groupCount");
            int requiredArraySize = Math.max(1, groupCount);
            if (groupMasks == null || requiredArraySize != groupMasks.length) {
                groupMasks = new GROUP_AFFINITY[requiredArraySize];
            }
            // Now read the fields from field order
            super.read();
            // Finally copy first value from array to older version of structure
            groupMask = groupMasks[0];
        }

Whether that is more efficient, that simple rebuilding the array from scratch and letting the GC do the work is a different discussion. The GC might win here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think groupMasks == null will ever happen. We need a minimum array size of 1 and initialize it that way. Reading only needs to resize it if it needs to grow. (And maybe shrink on re-read.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the null check, what you have is essentially what I proposed (And could also do for PROCESSOR_RELATIONSHIP.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'm done tweaking now.

contrib/platform/src/com/sun/jna/platform/win32/WinNT.java Outdated Show resolved Hide resolved
@dbwiddis dbwiddis force-pushed the numa branch 3 times, most recently from 0815655 to 33e0fc4 Compare August 10, 2021 23:08
Copy link
Member

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants