-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
cc3a87a
to
901e443
Compare
12dc9a3
to
bf297aa
Compare
if (groupCount > 1) { | ||
groupMasks = new GROUP_AFFINITY[groupCount]; | ||
} |
There was a problem hiding this comment.
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:
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.
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];
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
.)
There was a problem hiding this comment.
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.
0815655
to
33e0fc4
Compare
There was a problem hiding this 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.
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.