92 lines
3.6 KiB
Markdown
92 lines
3.6 KiB
Markdown
# Refactoring review
|
|
|
|
This is the remaining refactoring backlog after PR #61. Completed work is
|
|
intentionally omitted. Any implementation must preserve DS4 behavior,
|
|
especially model execution, token processing, context accounting, and KV-cache
|
|
handling.
|
|
|
|
## Current assessment
|
|
|
|
The codebase remains deliberate, concrete Rust. Database message invariants,
|
|
runtime submission, checkpoint/source modeling, preference dispatch, and the
|
|
pure A2UI evaluation and validation boundaries are now concentrated in clear
|
|
locations.
|
|
|
|
The remaining concentration is mostly in `App::update` and the Metal execution
|
|
path. Neither justifies a broad framework: keep extracting only boundaries that
|
|
already exist in the domain, and treat Metal changes as high-risk DS4 parity
|
|
work.
|
|
|
|
## Remaining work
|
|
|
|
### 1. Continue reducing `App::update` at owned domain boundaries
|
|
|
|
Priority: medium
|
|
|
|
Preference messages now dispatch through `src/app/preferences.rs`, but
|
|
`App::update` still spans roughly 890 lines and handles window lifecycle,
|
|
projects, generation, A2UI, Git, model management, cache management, and native
|
|
integration.
|
|
|
|
Move another coherent message family only when its existing `src/app/*` module
|
|
can own the complete handling flow. Keep the single public Iced update entry
|
|
point. Avoid controller objects and generic dispatch infrastructure; with the
|
|
current flat `Message` enum, extra pass-through matchers also weaken exhaustive
|
|
matching and should earn that cost.
|
|
|
|
### 2. Concentrate repeated Metal FFI invariants incrementally
|
|
|
|
Priority: medium, high risk
|
|
|
|
`src/engine/metal.rs` and `src/engine/metal/glm.rs` still contain many direct
|
|
unsafe calls inside high-level execution logic. `src/engine/metal/gpu.rs`
|
|
already proves that small wrappers can own repeated tensor, command, and
|
|
lifecycle invariants, but coverage is incomplete.
|
|
|
|
Add safe wrappers only for repeated, proven operations. Do not introduce a
|
|
general GPU framework or reorganize execution merely to reduce unsafe-block
|
|
count. Every change here requires DS4 parity coverage and the relevant real
|
|
model/Metal fixtures.
|
|
|
|
### 3. Replace argument-count suppressions only for existing records
|
|
|
|
Priority: low
|
|
|
|
Most remaining suppressions mirror explicit tensor/kernel operands or recursive
|
|
A2UI traversal state and should stay. The clearest remaining candidate is
|
|
metrics publication: `Metrics::ssd_stats` accepts nineteen fields that already
|
|
travel together as execution statistics, and its call is duplicated in
|
|
`src/engine.rs`.
|
|
|
|
Reuse or introduce one concrete statistics record only if it removes that
|
|
duplication without coupling metrics to the Metal executor. Do not create
|
|
one-use parameter objects for `Generator::generate`, `Generator::compact`,
|
|
Metal kernels, or A2UI rendering solely to satisfy Clippy.
|
|
|
|
## Leave alone
|
|
|
|
### Generated hotlist data
|
|
|
|
`src/engine/metal/hotlist.rs` is mechanically generated. Deterministic embedded
|
|
data with its import script is preferable to runtime machinery added only to
|
|
reduce source size.
|
|
|
|
### Preference-to-engine conversion
|
|
|
|
The preference types and effective engine-setting types in `src/settings.rs`
|
|
form a useful normalization boundary: optional user input is validated and
|
|
converted into concrete DS4-compatible runtime values.
|
|
|
|
### Concrete model descriptions
|
|
|
|
The model `Shape` constants in `src/engine.rs` reuse common values with struct
|
|
update syntax while keeping model differences explicit.
|
|
|
|
## Suggested order
|
|
|
|
1. Extract one more naturally owned `App::update` message family.
|
|
2. Consolidate SSD metrics publication if a single concrete record keeps the
|
|
boundary simpler.
|
|
3. Tighten repeated Metal FFI operations only alongside dedicated DS4 parity
|
|
tests and real fixture runs.
|