-
Notifications
You must be signed in to change notification settings - Fork 801
Fix XNNWorkspaceManager cleanup tests by using unique IDs instead of raw pointers #16675
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
base: main
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16675
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 Cancelled Jobs, 6 Unrelated FailuresAs of commit 9591f72 with merge base d305410 ( CANCELLED JOBS - The following jobs were cancelled. Please retry:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but was present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@mergennachin has exported this pull request. If you are a Meta employee, you can view the originating Diff in D90897333. |
This PR needs a
|
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.
Pull request overview
This PR fixes failing workspace manager cleanup tests by introducing unique instance IDs to distinguish between workspace objects, solving the issue where memory allocators reuse addresses after deallocation.
Changes:
- Added a unique ID mechanism to XNNWorkspace using an atomic counter
- Modified cleanup tests to compare workspace IDs instead of raw pointers
- Added explanatory comments documenting why ID comparison is used
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backends/xnnpack/runtime/XNNWorkspace.h | Added atomic counter for unique IDs, id() getter method, and id_ member variable to XNNWorkspace class |
| backends/xnnpack/test/runtime/test_workspace_manager.cpp | Updated PerModelModeCleanup and GlobalModeCleanup tests to compare workspace IDs instead of raw pointers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…raw pointers (pytorch#16675) Summary: The PerModelModeCleanup and GlobalModeCleanup tests were failing because they compared raw xnn_workspace_t pointers to verify cleanup behavior. However, memory allocators commonly reuse addresses after deallocation - when xnn_release_workspace frees memory and xnn_create_workspace immediately allocates the same size, the allocator may return the same address. This fix: 1. Adds a unique instance ID (uint64_t) to XNNWorkspace using an atomic counter 2. Adds an id() method to retrieve this unique ID 3. Modifies both cleanup tests to compare IDs instead of raw pointers This correctly verifies that new workspace objects are created after cleanup, regardless of memory address reuse by the allocator. Differential Revision: D90897333
6614645 to
36df5d3
Compare
…raw pointers (pytorch#16675) Summary: The PerModelModeCleanup and GlobalModeCleanup tests were failing because they compared raw xnn_workspace_t pointers to verify cleanup behavior. However, memory allocators commonly reuse addresses after deallocation - when xnn_release_workspace frees memory and xnn_create_workspace immediately allocates the same size, the allocator may return the same address. This fix: 1. Adds a unique instance ID (uint64_t) to XNNWorkspace using an atomic counter 2. Adds an id() method to retrieve this unique ID 3. Modifies both cleanup tests to compare IDs instead of raw pointers This correctly verifies that new workspace objects are created after cleanup, regardless of memory address reuse by the allocator. Differential Revision: D90897333
36df5d3 to
9591f72
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary:
The PerModelModeCleanup and GlobalModeCleanup tests were failing because they
compared raw xnn_workspace_t pointers to verify cleanup behavior. However,
memory allocators commonly reuse addresses after deallocation - when
xnn_release_workspace frees memory and xnn_create_workspace immediately
allocates the same size, the allocator may return the same address.
This fix:
This correctly verifies that new workspace objects are created after cleanup,
regardless of memory address reuse by the allocator.
Differential Revision: D90897333