fix-deviant(renderer): don't over-allocate Metal buffers by sizeof(T)
Intentional deviation from Ghostty 1.3.1 (DEVIANT_FIXES.md row 34).
When a Metal buffer grows, Zig's renderer/metal/buffer.zig:89 and :146
compute the new size as twice the required *byte* count and then
multiply by @sizeOf(T) again, so large cell buffers allocate
sizeof(T) times more GPU memory than intended. The Rust Metal backend
(since bab49ee) allocates exactly twice the required bytes; this commit
factors the computation into metal_layout::grown_buffer_bytes, adds a
regression test and records the deviation.
Test Plan:
- cargo test -p ghostty-renderer
grown_buffer_bytes_is_double_the_required_bytes_not_scaled_by_size_of_t
- just check-macos
This commit is contained in:
1 parent
132f7c27ba
commit
2040677b05
3 files changed
+28
-3
No files matched your search
@@ -9,7 +9,7 @@ use objc2_metal::{MTLBuffer, MTLDevice as _, MTLResourceOptions};
|
|||||||
use super::{Device, Error};
|
use super::{Device, Error};
|
||||||
use crate::{
|
use crate::{
|
||||||
graphics_api::{ApiError, GpuBuffer},
|
graphics_api::{ApiError, GpuBuffer},
|
||||||
metal_layout::{ResourceOptions, StorageMode},
|
metal_layout::{ResourceOptions, StorageMode, grown_buffer_bytes},
|
||||||
};
|
};
|
||||||
|
|
||||||
/// Options for initializing a buffer.
|
/// Options for initializing a buffer.
|
||||||
@@ -124,8 +124,9 @@ impl<T: Pod> Buffer<T> {
|
|||||||
// require (the old buffer is released when it is replaced).
|
// require (the old buffer is released when it is replaced).
|
||||||
//
|
//
|
||||||
// NOTE: Zig multiplies this size by `@sizeOf(T)` a second time,
|
// NOTE: Zig multiplies this size by `@sizeOf(T)` a second time,
|
||||||
// over-allocating; we allocate exactly double.
|
// over-allocating; we allocate exactly double
|
||||||
let size = req_bytes * 2;
|
// (see `grown_buffer_bytes`).
|
||||||
|
let size = grown_buffer_bytes(req_bytes);
|
||||||
self.buffer = self
|
self.buffer = self
|
||||||
.opts
|
.opts
|
||||||
.device
|
.device
|
||||||
|
|||||||
@@ -478,6 +478,19 @@ pub const PIPELINE_DESCS: [PipelineDesc; 5] = [
|
|||||||
},
|
},
|
||||||
];
|
];
|
||||||
|
|
||||||
|
/// Size in bytes of the replacement Metal buffer when `req_bytes` bytes no
|
||||||
|
/// longer fit (`metal/buffer.zig` `sync`/`syncFromArrayLists`: "enough to
|
||||||
|
/// hold double what we require").
|
||||||
|
///
|
||||||
|
/// Upstream bug fixed: Zig computes `size = req_bytes * 2` and then passes
|
||||||
|
/// `size * @sizeOf(T)` to `newBufferWithLength:options:`, multiplying by the
|
||||||
|
/// element size a second time (`sizeof(T)` times too much GPU memory). The
|
||||||
|
/// length is already in bytes, so this is exactly double.
|
||||||
|
#[must_use]
|
||||||
|
pub const fn grown_buffer_bytes(req_bytes: usize) -> usize {
|
||||||
|
req_bytes * 2
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use core::mem::{offset_of, size_of};
|
use core::mem::{offset_of, size_of};
|
||||||
@@ -485,6 +498,16 @@ mod tests {
|
|||||||
use super::*;
|
use super::*;
|
||||||
use crate::shader_types::{Bools, PaddingExtend};
|
use crate::shader_types::{Bools, PaddingExtend};
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn grown_buffer_bytes_is_double_the_required_bytes_not_scaled_by_size_of_t() {
|
||||||
|
// 100 vertices of a 32-byte type: 3200 bytes required, 6400 allocated
|
||||||
|
// (Zig would ask Metal for 6400 * 32 bytes).
|
||||||
|
let req_bytes = 100 * size_of::<CellText>();
|
||||||
|
assert_eq!(grown_buffer_bytes(req_bytes), req_bytes * 2);
|
||||||
|
assert_eq!(grown_buffer_bytes(0), 0);
|
||||||
|
assert_eq!(grown_buffer_bytes(1), 2);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn resource_options_bits() {
|
fn resource_options_bits() {
|
||||||
// MTLResourceCPUCacheModeWriteCombined | MTLResourceStorageModeShared
|
// MTLResourceCPUCacheModeWriteCombined | MTLResourceStorageModeShared
|
||||||
|
|||||||
@@ -59,6 +59,7 @@ when this file was written.
|
|||||||
| 31 | GTK: `+new-window` D-Bus arguments | `apprt/gtk/ipc/new_window.zig:49-53` (`Bytes.new(ptr, len + 1)` + `Variant.newFromBytes(..., trusted = true)`) | Arguments are passed as trusted GVariant strings without validation; invalid UTF-8 or an interior NUL makes GIO abort the process while serializing (input-triggered crash). | `arguments_variant` validates first and returns `Error::IpcFailed` (message on stderr, no D-Bus call); valid arguments are byte-identical to Zig. | `65dd9d5` fix-deviant(gtk) | `ipc::new_window::tests::invalid_dbus_strings_are_rejected_without_abort` (`ghostty-apprt-gtk`) |
|
| 31 | GTK: `+new-window` D-Bus arguments | `apprt/gtk/ipc/new_window.zig:49-53` (`Bytes.new(ptr, len + 1)` + `Variant.newFromBytes(..., trusted = true)`) | Arguments are passed as trusted GVariant strings without validation; invalid UTF-8 or an interior NUL makes GIO abort the process while serializing (input-triggered crash). | `arguments_variant` validates first and returns `Error::IpcFailed` (message on stderr, no D-Bus call); valid arguments are byte-identical to Zig. | `65dd9d5` fix-deviant(gtk) | `ipc::new_window::tests::invalid_dbus_strings_are_rejected_without_abort` (`ghostty-apprt-gtk`) |
|
||||||
| 32 | OS: URL opener child reaping | `os/open.zig` (`try collectOutput(...)` before `wait()`) | When collecting the opener's output fails (e.g. more than 50 KiB on a stream), the error returns before `wait()`, leaving the child as a zombie. | The child is always reaped; the collection error is still logged. | `4a55abf` fix-deviant(os) | `ghostty-os` open tests |
|
| 32 | OS: URL opener child reaping | `os/open.zig` (`try collectOutput(...)` before `wait()`) | When collecting the opener's output fails (e.g. more than 50 KiB on a stream), the error returns before `wait()`, leaving the child as a zombie. | The child is always reaped; the collection error is still logged. | `4a55abf` fix-deviant(os) | `ghostty-os` open tests |
|
||||||
| 33 | OS: maximum-length `NOTIFY_SOCKET` | `os/systemd.zig:100-116` (`socket_address.path.len < socket_path.len`, then `path[socket_path.len] = 0`) | A 108-byte path (the size of `sockaddr_un.path`) passes the length check, and the terminating NUL is written at `path[108]`, one byte past the array (out-of-bounds write). Separate from row 13 (the abstract-socket `@` handling). | The address is built with rustix's `SocketAddrUnix::new`, which takes the 108 bytes without a terminator (valid for Linux `AF_UNIX`) and rejects longer paths; no out-of-bounds write. Verified in code. | f8c5ce0 (`git log -S` on `ghostty-os/src/systemd.rs`: the only commit touching it, besides formatting/macOS changes) | `notify_maximum_length_socket_path_is_rejected_safely` (`ghostty-os/src/systemd.rs`) |
|
| 33 | OS: maximum-length `NOTIFY_SOCKET` | `os/systemd.zig:100-116` (`socket_address.path.len < socket_path.len`, then `path[socket_path.len] = 0`) | A 108-byte path (the size of `sockaddr_un.path`) passes the length check, and the terminating NUL is written at `path[108]`, one byte past the array (out-of-bounds write). Separate from row 13 (the abstract-socket `@` handling). | The address is built with rustix's `SocketAddrUnix::new`, which takes the 108 bytes without a terminator (valid for Linux `AF_UNIX`) and rejects longer paths; no out-of-bounds write. Verified in code. | f8c5ce0 (`git log -S` on `ghostty-os/src/systemd.rs`: the only commit touching it, besides formatting/macOS changes) | `notify_maximum_length_socket_path_is_rejected_safely` (`ghostty-os/src/systemd.rs`) |
|
||||||
|
| 34 | Renderer (Metal): buffer reallocation size | `renderer/metal/buffer.zig:89` (`Buffer.sync`) and `:146` (`syncFromArrayLists`) | `size = req_bytes * 2` is already in bytes, then `size * @sizeOf(T)` is passed to `newBufferWithLength:options:`, so the new buffer is `sizeof(T)` times larger than intended (GPU-memory over-allocation). | The new buffer is exactly `2 * req_bytes` (`metal_layout::grown_buffer_bytes`); the element count is `2 * total_len`. | bab49ee (`git log -S"over-allocating"` on `ghostty-renderer/src/metal/buffer.rs`; the file's first commit) | `metal_layout::tests::grown_buffer_bytes_is_double_the_required_bytes_not_scaled_by_size_of_t` (runs on any host; the Metal buffer itself is compile-checked only) |
|
||||||
|
|
||||||
## Upstream quirks deliberately kept (not fixed)
|
## Upstream quirks deliberately kept (not fixed)
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user