witness: add optional device backing metadata to RowMajorMatrix#33
witness: add optional device backing metadata to RowMajorMatrix#33
Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional device-backing metadata to witness::RowMajorMatrix so callers can attach an opaque device storage handle plus a layout tag.
Changes:
- Add
device_backing: Option<…>toRowMajorMatrixand initialize it toNonein constructors. - Introduce
DeviceMatrixLayoutand internalDeviceMatrixBacking. - Add public APIs to set/clear/query/downcast the device backing.
| pub fn set_device_backing<D: Any + Send + Sync + 'static>( | ||
| &mut self, | ||
| storage: D, | ||
| layout: DeviceMatrixLayout, | ||
| ) { | ||
| self.device_backing = Some(DeviceMatrixBacking { | ||
| storage: Arc::new(storage), | ||
| layout, | ||
| }); | ||
| } |
There was a problem hiding this comment.
device_backing can become silently stale: RowMajorMatrix exposes multiple mutable access paths (e.g., DerefMut and the various *_mut iterators/padding methods) but setting a device backing doesn’t establish/maintain any invariant that it matches the current host-side inner.values. If callers set a device backing and then mutate the matrix, downstream consumers may read an out-of-sync device buffer. Consider defining the invariant explicitly and either (a) automatically invalidating device_backing on any host mutation (clear on deref_mut / mut iterators / padding / pad_to_height), or (b) restricting mutable access when a backing is present and forcing callers to clear/update it first.
| pub fn device_backing_ref<D: Any + 'static>(&self) -> Option<&D> { | ||
| self.device_backing | ||
| .as_ref() | ||
| .and_then(|backing| backing.storage.downcast_ref::<D>()) | ||
| } |
There was a problem hiding this comment.
device_backing_ref accepts D: Any + 'static, but set_device_backing only permits storing D: Any + Send + Sync + 'static. This mismatch makes it easy to ask for a non-Send + Sync type that can never be present, and is inconsistent with the setter’s contract. Consider mirroring the bounds (D: Any + Send + Sync + 'static) or documenting the intended usage to avoid confusing API calls that always return None.
| is_padded: bool, | ||
| padding_strategy: InstancePaddingStrategy, | ||
| device_backing: Option<DeviceMatrixBacking>, | ||
| } |
There was a problem hiding this comment.
RowMajorMatrix derives Clone, and the new device_backing: Option<DeviceMatrixBacking> will be cloned as well (cloning the inner Arc). Since inner is also cloned, the cloned matrix will share the same device backing metadata as the original even though the host-side values are a deep copy, which is very likely to make the backing inconsistent/wrong. Consider implementing a manual Clone for RowMajorMatrix that resets device_backing to None (or otherwise ensures the backing corresponds to the cloned contents).
| pub fn clear_device_backing(&mut self) { | ||
| self.device_backing = None; | ||
| } | ||
|
|
||
| pub fn has_device_backing(&self) -> bool { | ||
| self.device_backing.is_some() | ||
| } |
There was a problem hiding this comment.
There’s no invalidation mechanism tying device_backing to the current host-side contents/dimensions: the matrix can be mutated via existing APIs (e.g., DerefMut, iter_mut, pad_to_height, padding_by_strategy, etc.) without clearing or updating device_backing. That can leave stale device metadata attached and risks later code using the wrong device buffer/layout. Consider clearing device_backing in mutating methods (or tracking a version/dirty flag and requiring callers to refresh the backing when dirty).
PR still WIP
Copilot Reviewer Directive (keep this section)
When Copilot reviews this PR, apply
.github/copilot-instructions.mdstrictly.[severity] path:line (symbol)format.PR metadata: descriptionas a finding.