-
Notifications
You must be signed in to change notification settings - Fork 146
fixed DSiGetLogicalProcessorInformationEx and LoadNumaInfo #210
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
Conversation
Pull Request Review: Fixed DSiGetLogicalProcessorInformationEx and LoadNumaInfoSummaryThis PR fixes issues with Code Quality and Best PracticesPositive aspects:
Areas for improvement:
Potential Bugs and IssuesCritical Issue - Line 7685 in DSiWin32.pas: for iGroup := 0 to High(pInfo.Processor.GroupMask) do
pInfo.Processor.GroupMask[iGroup] := pInfo.Processor.GroupMask[iGroup];This line assigns a value to itself, which is a no-op. It should likely copy from pInfo.Processor.GroupMask[iGroup] := pBuffer.Processor.GroupMask[iGroup];This is a critical bug that will result in uninitialized data in the GroupMask array. Minor Issues:
if (not Result) and (GetLastError = ERROR_INSUFFICIENT_BUFFER) then
Performance ConsiderationsGood:
Concerns:
Security ConcernsLow Risk:
Recommendations:
Test CoverageConcerns:
Recommendations:
Action ItemsMust Fix:
Should Consider:
Testing Checklist:
Overall AssessmentThe refactoring improves code maintainability by replacing manual pointer arithmetic with cleaner array-based access. However, there is a critical bug in the GroupMask copying logic that must be fixed before merging. Additionally, test coverage should be added to prevent similar issues in the future. The new E-core/P-core detection features are a valuable addition for modern Intel hybrid architectures, but would benefit from better documentation and testing. |
Pull Request ReviewThanks for this PR! I've reviewed the changes and have the following feedback: ✅ Positive Changes
|
Pull Request ReviewThank you for these improvements to the logical processor information handling! This PR addresses several important fixes and adds useful functionality for modern CPUs with efficiency cores. Overall, the changes look solid, but I have some observations and suggestions. Code Quality & Best PracticesPositive aspects:
Areas for improvement:
Potential Bugs & Issues
Performance Considerations
Security ConcernsNo significant security issues identified. The code properly:
Test CoverageThe PR doesn't include explicit unit tests for the new E-Core/P-Core detection functions. Consider adding tests that:
Since this is low-level CPU detection, testing might require different hardware configurations or mocking the Windows API responses. DocumentationThe new functions have minimal documentation comments. Consider adding more detailed documentation for:
Minor Issues
SummaryThis is a solid PR that fixes important bugs and adds useful functionality. The main concerns are:
The core functionality appears sound, but testing on various CPU configurations (Intel with E-cores, AMD, older Intel) would be valuable to ensure correctness. Recommendation: Approve with minor changes requested for the documentation and potential cleanup of the committed local files. |
Pull Request Review: Fixed DSiGetLogicalProcessorInformationEx and LoadNumaInfoSummaryThis PR fixes a critical bug in Positive AspectsCode Quality
New Functionality
Issues and Concerns1. Memory Leak Risk (Minor - already handled correctly)DSiWin32.pas:7667-7698 - The GetMem(buffer, bufSize);
try
// ... operations
finally FreeMem(buffer); end;This is properly implemented. Good defensive programming. 2. Potential Type Confusion (Low severity)DSiWin32.pas:7676-7694 - The code handles multiple relationship types but doesn't have explicit handling for unrecognized types: case pInfo.Relationship of
RelationProcessorCore, RelationProcessorDie, RelationProcessorModule, RelationProcessorPackage:
// handled
RelationNumaNode, RelationNumaNodeEx:
// handled
RelationCache:
// handled
RelationGroup:
// handled
// else: What happens with future relationship types?
end;Recommendation: Consider adding an 3. Array Indexing Concern (Medium severity)DSiWin32.pas:7673-7674: SetLength(info, Length(info) + 1);
pInfo := @info[Length(info)-1];This pattern of repeatedly growing an array is inefficient (O(n²) complexity for n items). Recommendation: Consider pre-allocating or using a growth strategy. You could estimate the size by dividing 4. Range Check Disable (Minor security concern)DSiWin32.pas:7683-7686: {$R-}
for iGroup := 0 to High(pInfo.Processor.GroupMask) do
pInfo.Processor.GroupMask[iGroup] := pBuffer.Processor.GroupMask[iGroup];
{$IFDEF RestoreR}{$R+}{$ENDIF}Concern: Range checking is disabled, which could mask buffer overrun bugs if Recommendation: Add an assertion or bounds check before the loop: Assert(pInfo.Processor.GroupCount <= Length(pBuffer.Processor.GroupMask));5. E-core/P-core Bit Manipulation Logic (Potential bug)DSiWin32.pas:5910-5918 and DSiWin32.pas:5940-5948: for iProcInfo := High(info) downto 0 do begin // get bits in correct order
bit := 0;
if info[iProcInfo].Processor.EfficiencyClass = 0 then
bit := 1;
if info[iProcInfo].Processor.Flags = 0 then // normal core
Result := Result SHL 1 OR bit
else // hyperthreaded core
Result := (Result SHL 1 OR bit) SHL 1 OR bit;
end;Concerns:
Recommendation:
6. Error Handling GapOtlCommon.pas:4374-4377: DSiGetLogicalProcessorInfoEx(DSiWin32._LOGICAL_PROCESSOR_RELATIONSHIP.RelationAll, procInfo);
if GetLastError = ERROR_NOT_SUPPORTED then
CreateFakeNUMAInfo
else beginIssue: This assumes that if Recommendation: Check the return value explicitly: if not DSiGetLogicalProcessorInfoEx(...) then begin
if GetLastError = ERROR_NOT_SUPPORTED then
CreateFakeNUMAInfo
else
raise Exception.CreateFmt('LoadNUMAInfo: Failed to get processor info [%d] %s',
[GetLastError, SysErrorMessage(GetLastError)]);
endPerformance ConsiderationsPositive
Area for Improvement
Security AssessmentLow Risk Overall:
Minor concerns:
Test CoverageConcern: This PR includes no new tests for:
Existing test: test_64_ProcessorGroups_NUMA.pas likely exercises Recommendation: Add unit tests for:
Additional Notes
Recommendations SummaryMust Fix:
Should Fix:
Consider:
ConclusionThis is a solid improvement that fixes a real bug and adds useful functionality for modern CPUs. The refactoring makes the code cleaner and more maintainable. With the recommended fixes above, particularly the error handling and array growth optimization, this would be excellent code. The new E-core/P-core functionality is particularly valuable for applications that want to optimize thread placement on hybrid Intel architectures. Status: Approve with minor recommendations for follow-up improvements. |
|
Bito Review Skipped - No Changes Detected |
Summary by Bito