Module deep-copies LoadBackendOptionsMap on load (#19673)#19673
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19673
Note: Links to docs will display an error until the docs builds have been completed. ❗ 2 Active SEVsThere are 2 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit cba9369 with merge base f3387d0 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@metascroy has exported this pull request. If you are a Meta employee, you can view the originating Diff in D105100372. |
This PR needs a
|
Summary: Module::load(LoadBackendOptionsMap&, ...) previously stored a raw const-pointer to the caller's map (backend_options_), forcing every caller (and every language binding) to keep the input map and its backing BackendOption arrays alive past the load() call to keep lazy load_method invocations safe. That contract is hard to express across language boundaries (it's the reason the Apple bindings carry an extra ivar to retain the wrapper -- removed in a follow-up). Replace the borrowed-pointer with a Module-owned copy: - Module gains a (storage, owned map, has-flag) triple replacing the raw pointer. The storage is a vector-of-vectors that owns the BackendOption arrays; the owned map's Spans reference it. - load(LoadBackendOptionsMap&, ...) deep-copies via the new LoadBackendOptionsMap::entry_at(i) accessor into a local (storage, map) pair, calls load_internal, and only commits to the members on success. Transactional rollback for free. - load_method's fallback (when called with no explicit options) now reads the Module-owned map. The signature of the public load(LoadBackendOptionsMap&, ...) overload is unchanged; existing callers (XNNPACK runtime tests, dynamic_shim, module_test) get the upgrade for free and may now release their input map immediately after load() returns. Reviewed By: shoumikhin Differential Revision: D105100372
54809fc to
b12afa4
Compare
Summary: Module::load(LoadBackendOptionsMap&, ...) previously stored a raw const-pointer to the caller's map (backend_options_), forcing every caller (and every language binding) to keep the input map and its backing BackendOption arrays alive past the load() call to keep lazy load_method invocations safe. That contract is hard to express across language boundaries (it's the reason the Apple bindings carry an extra ivar to retain the wrapper -- removed in a follow-up). Replace the borrowed-pointer with a Module-owned copy: - Module gains a (storage, owned map, has-flag) triple replacing the raw pointer. The storage is a vector-of-vectors that owns the BackendOption arrays; the owned map's Spans reference it. - load(LoadBackendOptionsMap&, ...) deep-copies via the new LoadBackendOptionsMap::entry_at(i) accessor into a local (storage, map) pair, calls load_internal, and only commits to the members on success. Transactional rollback for free. - load_method's fallback (when called with no explicit options) now reads the Module-owned map. The signature of the public load(LoadBackendOptionsMap&, ...) overload is unchanged; existing callers (XNNPACK runtime tests, dynamic_shim, module_test) get the upgrade for free and may now release their input map immediately after load() returns. Reviewed By: shoumikhin Differential Revision: D105100372
b12afa4 to
1cb79b4
Compare
Summary: Module::load(LoadBackendOptionsMap&, ...) previously stored a raw const-pointer to the caller's map (backend_options_), forcing every caller (and every language binding) to keep the input map and its backing BackendOption arrays alive past the load() call to keep lazy load_method invocations safe. That contract is hard to express across language boundaries (it's the reason the Apple bindings carry an extra ivar to retain the wrapper -- removed in a follow-up). Replace the borrowed-pointer with a Module-owned copy: - Module gains a (storage, owned map, has-flag) triple replacing the raw pointer. The storage is a vector-of-vectors that owns the BackendOption arrays; the owned map's Spans reference it. - load(LoadBackendOptionsMap&, ...) deep-copies via the new LoadBackendOptionsMap::entry_at(i) accessor into a local (storage, map) pair, calls load_internal, and only commits to the members on success. Transactional rollback for free. - load_method's fallback (when called with no explicit options) now reads the Module-owned map. The signature of the public load(LoadBackendOptionsMap&, ...) overload is unchanged; existing callers (XNNPACK runtime tests, dynamic_shim, module_test) get the upgrade for free and may now release their input map immediately after load() returns. Reviewed By: shoumikhin Differential Revision: D105100372
1cb79b4 to
cba9369
Compare
Summary:
Module::load(LoadBackendOptionsMap&, ...) previously stored a raw
const-pointer to the caller's map (backend_options_), forcing every
caller (and every language binding) to keep the input map and its
backing BackendOption arrays alive past the load() call to keep lazy
load_method invocations safe. That contract is hard to express across
language boundaries (it's the reason the Apple bindings carry an extra
ivar to retain the wrapper -- removed in a follow-up).
Replace the borrowed-pointer with a Module-owned copy:
raw pointer. The storage is a vector-of-vectors that owns the
BackendOption arrays; the owned map's Spans reference it.
LoadBackendOptionsMap::entry_at(i) accessor into a local
(storage, map) pair, calls load_internal, and only commits to the
members on success. Transactional rollback for free.
reads the Module-owned map.
The signature of the public load(LoadBackendOptionsMap&, ...) overload
is unchanged; existing callers (XNNPACK runtime tests, dynamic_shim,
module_test) get the upgrade for free and may now release their
input map immediately after load() returns.
Reviewed By: shoumikhin
Differential Revision: D105100372