Update NUMA_NODE_RELATIONSHIP to new variable size structure#1363
Update NUMA_NODE_RELATIONSHIP to new variable size structure#1363dbwiddis merged 1 commit intojava-native-access:masterfrom
Conversation
cc3a87a to
901e443
Compare
12dc9a3 to
bf297aa
Compare
| if (groupCount > 1) { | ||
| groupMasks = new GROUP_AFFINITY[groupCount]; | ||
| } |
There was a problem hiding this comment.
I suggest to replace with:
| 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.
There was a problem hiding this comment.
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];
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Other than the null check, what you have is essentially what I proposed (And could also do for PROCESSOR_RELATIONSHIP.)
There was a problem hiding this comment.
OK, I'm done tweaking now.
0815655 to
33e0fc4
Compare
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.