Xnnpack disable workspace nonlock by Froskekongen · Pull Request #17780 · pytorch/executorch

Froskekongen · 2026-03-02T12:13:29Z

Summary

Remove lock on XNNPACK Disabled Workspace Mode.

Test plan

See test.

cc @GregoryComer @digantdesai @cbilgin

pytorch-bot · 2026-03-02T12:13:33Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/17780

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⚠️ 8 Awaiting Approval

As of commit 3871367 with merge base 9aab9e7 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

meta-cla · 2026-03-02T12:13:35Z

Hi @Froskekongen!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

Froskekongen · 2026-03-02T12:16:52Z

See #17301

Froskekongen · 2026-03-02T12:28:59Z

@pytorchbot label "release notes: When Disable Workspace mode is enabled for XNNPack, execute does not lock."

pytorch-bot · 2026-03-02T12:29:03Z

Didn't find following labels among repository labels: release notes: When Disable Workspace mode is enabled for XNNPack, execute does not lock.

GregoryComer · 2026-03-02T20:56:18Z

backends/xnnpack/runtime/XNNWorkspaceManager.cpp

      return create_result.error();
    }

+    create_result.get()->disable_locking();


Nit: Can we pass this in as a arg to XNNWorkspace::create?

GregoryComer

Waiting for CI to run, but I'm okay to merge if it's green.

Froskekongen · 2026-03-04T08:44:37Z

@GregoryComer lint error should be fixed now.

Copilot

Pull request overview

This PR removes the mutex lock from XNNWorkspace::acquire() when workspace sharing is in Disabled mode (each delegate instance has its own workspace with no concurrent access). Since no sharing occurs in Disabled mode, the lock is unnecessary overhead. The change adds a lock_required_ flag set via a new disable_locking() method, called immediately after workspace creation for Disabled mode.

Changes:

XNNWorkspace.h: Adds lock_required_ flag and disable_locking() method; acquire() skips locking when lock_required_ is false.
XNNWorkspaceManager.cpp: Calls disable_locking() on newly-created workspaces when in Disabled mode.
test_workspace_manager.cpp / CMakeLists.txt: Adds two new tests verifying locking behavior per mode, and registers the test file in the build.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`backends/xnnpack/runtime/XNNWorkspace.h`	Adds `lock_required_` member and `disable_locking()` public method; modifies `acquire()` to skip locking when flag is false
`backends/xnnpack/runtime/XNNWorkspaceManager.cpp`	Calls `disable_locking()` on workspace after creation in `Disabled` mode
`backends/xnnpack/test/runtime/test_workspace_manager.cpp`	Adds `DisabledModeAcquireDoesNotLock` and `PerModelAcquireStillLocks` tests
`backends/xnnpack/test/CMakeLists.txt`	Adds `test_workspace_manager.cpp` to the test build target

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-04T15:24:37Z

backends/xnnpack/runtime/XNNWorkspace.h

+  void disable_locking() {
+    lock_required_ = false;
+  }


The lock_required_ field is a plain bool that is written by disable_locking() and read by acquire(). Although the current single call site in XNNWorkspaceManager.cpp calls disable_locking() before the workspace is shared with any other thread (making it safe in practice), the public disable_locking() method has no documented threading constraints, and calling it on an already-shared workspace while acquire() is being called concurrently from another thread would be an undefined behavior data race. Consider making lock_required_ a std::atomic<bool> to ensure correctness regardless of call order, or at minimum add a comment documenting the thread-safety requirement (that it must only be called before the workspace is shared with other threads).

Copilot · 2026-03-04T15:24:37Z

backends/xnnpack/test/runtime/test_workspace_manager.cpp

+TEST_F(XNNWorkspaceManagerTest, PerModelAcquireStillLocks) {
+  workspace_manager_->set_sharing_mode(WorkspaceSharingMode::PerModel);
+
+  auto workspace_result = workspace_manager_->get_or_create_workspace(12345);
+  ASSERT_TRUE(workspace_result.ok());
+  auto workspace = workspace_result.get();
+
+  auto [lock, ptr] = workspace->acquire();
+  ASSERT_NE(ptr, nullptr);
+  EXPECT_TRUE(lock.owns_lock());
+}


The PR adds DisabledModeAcquireDoesNotLock and PerModelAcquireStillLocks to verify locking behavior per mode, but there is no corresponding GlobalAcquireStillLocks test for the Global sharing mode. Since Global mode shares a workspace across all callers and also relies on the mutex to serialize access, adding a test verifying that acquire() in Global mode returns a locked unique_lock would make the test suite complete and symmetric.

Froskekongen added 2 commits March 2, 2026 12:42

Remove lock when Disabled Workspace mode is set.

21d9bdd

Run xnnpack workspace disable/enable lock test

7f4bb96

Froskekongen requested review from digantdesai, kirklandsign and larryliu0820 as code owners March 2, 2026 12:13

Merge branch 'main' into xnnpack-disable-workspace-nonlock

ccdbca7

meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 2, 2026

Froskekongen mentioned this pull request Mar 2, 2026

XNNPACK Disabled Workspace Mode Still Locks in execute() (fix patch included in issue) #17301

Open

GregoryComer self-assigned this Mar 2, 2026

GregoryComer self-requested a review March 2, 2026 19:37

GregoryComer added module: xnnpack Issues related to xnnpack delegation and the code under backends/xnnpack/ release notes: xnnpack Changes to the XNNPack backend delegate labels Mar 2, 2026

GregoryComer reviewed Mar 2, 2026

View reviewed changes

GregoryComer approved these changes Mar 2, 2026

View reviewed changes

Froskekongen added 2 commits March 4, 2026 09:42

Fix lint error

019327b

Merge branch 'main' into xnnpack-disable-workspace-nonlock

3871367

kirklandsign requested review from Copilot and removed request for digantdesai, kirklandsign and larryliu0820 March 4, 2026 15:21

Copilot started reviewing on behalf of kirklandsign March 4, 2026 15:22 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xnnpack disable workspace nonlock#17780

Xnnpack disable workspace nonlock#17780
Froskekongen wants to merge 5 commits intopytorch:mainfrom
Froskekongen:xnnpack-disable-workspace-nonlock

Froskekongen commented Mar 2, 2026 •

edited by pytorch-bot bot

Loading

Uh oh!

pytorch-bot bot commented Mar 2, 2026 •

edited

Loading

Uh oh!

meta-cla bot commented Mar 2, 2026

Uh oh!

Froskekongen commented Mar 2, 2026

Uh oh!

Froskekongen commented Mar 2, 2026

Uh oh!

pytorch-bot bot commented Mar 2, 2026

Uh oh!

GregoryComer Mar 2, 2026

Uh oh!

GregoryComer left a comment

Uh oh!

Froskekongen commented Mar 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 4, 2026

Uh oh!

Copilot AI Mar 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Froskekongen commented Mar 2, 2026 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

pytorch-bot bot commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/17780

⚠️ 8 Awaiting Approval

Uh oh!

meta-cla bot commented Mar 2, 2026

Action Required

Process

Uh oh!

Froskekongen commented Mar 2, 2026

Uh oh!

Froskekongen commented Mar 2, 2026

Uh oh!

pytorch-bot bot commented Mar 2, 2026

Uh oh!

GregoryComer Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

GregoryComer left a comment

Choose a reason for hiding this comment

Uh oh!

Froskekongen commented Mar 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Froskekongen commented Mar 2, 2026 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Mar 2, 2026 •

edited

Loading