diff options
| author | skal <pascal.massimino@gmail.com> | 2026-02-06 10:16:50 +0100 |
|---|---|---|
| committer | skal <pascal.massimino@gmail.com> | 2026-02-06 10:16:50 +0100 |
| commit | e281b6ff0884a5b4af8aa2ca79fd01141bc2005b (patch) | |
| tree | b6c33df6a2667fc4852b277275765eea15bac89f /doc/BUILD_OPTIMIZATION_PROPOSAL_V2.md | |
| parent | 98ab690a5e6c5b038a3842dd071848c0059971ae (diff) | |
refactor(build): Split asset_manager.h into dcl/core/utils headers
Split monolithic asset_manager.h (61 lines) into 3 focused headers:
- asset_manager_dcl.h: Forward declarations (AssetId, ProcGenFunc)
- asset_manager.h: Core API (GetAsset, DropAsset, AssetRecord)
- asset_manager_utils.h: Typed helpers (TextureAsset, MeshAsset)
Updated 17 source files to use appropriate headers:
- object.h: Uses dcl.h (only needs AssetId forward declaration)
- 7 files using TextureAsset/MeshAsset: Use utils.h
- 10 files using only GetAsset(): Keep asset_manager.h
Performance improvement:
- Before: Touch asset_manager.h → 4.82s (35 files rebuild)
- After: Touch asset_manager_utils.h → 2.01s (24 files rebuild)
- Improvement: 58% faster for common workflow (tweaking mesh/texture helpers)
Note: Touching base headers (dcl/core) still triggers ~33 file rebuilds
due to object.h dependency chain. Further optimization would require
reducing object.h's footprint (separate task).
Files changed:
- Created: asset_manager_dcl.h, asset_manager_utils.h
- Modified: asset_manager.h (removed structs), asset_manager.cc
- Updated: object.h, visual_debug.h, renderer_mesh.cc,
flash_cube_effect.cc, hybrid_3d_effect.cc, test files
Diffstat (limited to 'doc/BUILD_OPTIMIZATION_PROPOSAL_V2.md')
| -rw-r--r-- | doc/BUILD_OPTIMIZATION_PROPOSAL_V2.md | 346 |
1 files changed, 346 insertions, 0 deletions
diff --git a/doc/BUILD_OPTIMIZATION_PROPOSAL_V2.md b/doc/BUILD_OPTIMIZATION_PROPOSAL_V2.md new file mode 100644 index 0000000..315911a --- /dev/null +++ b/doc/BUILD_OPTIMIZATION_PROPOSAL_V2.md @@ -0,0 +1,346 @@ +# Build Optimization Proposal V2: Core Library Iteration Speed + +## Executive Summary (Revised Focus) + +**Real Problem**: Changing common headers during development causes **4-5 second rebuilds** of 35+ files. Asset changes are infrequent and acceptable. + +**Root Cause**: `asset_manager.h` is over-included. 17 files include it, but only 7 need the struct definitions (TextureAsset, MeshAsset). + +**Solution**: Split `asset_manager.h` into core + extensions → reduce rebuild cascade from **35 files to ~7 files**. + +--- + +## Revised Performance Baseline + +### Developer Workflow Measurements +``` +Single .cc file change: 0.65s ✅ (excellent) +Touch asset_manager.h: 4.82s ❌ (35 files rebuild) +Touch mini_math.h: 1.90s ⚠️ (23 files rebuild) +Touch object.h: ??? (needs measurement) +Asset changes: 3.50s ✅ (acceptable - rare) +``` + +**Key Insight**: The bottleneck is **header dependencies**, not asset generation. + +--- + +## Problem Analysis: asset_manager.h + +### Current Structure +```cpp +// asset_manager.h (MONOLITHIC) +#pragma once +#include <cstddef> +#include <cstdint> + +enum class AssetId : uint16_t; // ✅ Already forward-declared + +typedef bool (*ProcGenFunc)(...); + +struct AssetRecord { ... }; // ❌ Rarely needed + +const uint8_t* GetAsset(...); // ✅ Frequently needed +void DropAsset(...); // ✅ Frequently needed + +struct TextureAsset { ... }; // ❌ Only 2 files need this +struct MeshVertex { ... }; // ❌ Only 3 files need this +struct MeshAsset { ... }; // ❌ Only 3 files need this + +TextureAsset GetTextureAsset(...); // ❌ Only 2 files need this +MeshAsset GetMeshAsset(...); // ❌ Only 3 files need this +``` + +### Who Includes It? (17 files) +``` +✅ NEED BASIC (GetAsset only): + - src/3d/renderer_skybox.cc + - src/3d/renderer_sdf.cc + - src/audio/audio.cc + - src/audio/tracker.cc + - src/gpu/effects/shaders.cc + - src/audio/spectrogram_resource_manager.h + - ... (10 files total) + +❌ NEED STRUCTS (GetTextureAsset/GetMeshAsset): + - src/3d/renderer_mesh.cc (MeshAsset) + - src/3d/visual_debug.cc (MeshAsset) + - src/gpu/effects/flash_cube_effect.cc (TextureAsset) + - src/gpu/effects/hybrid_3d_effect.cc (TextureAsset) + - ... (7 files total) +``` + +### Transitive Includes (The Real Problem!) +``` +object.h + #include "asset_manager.h" ❌ Only needs AssetId (already forward-declared!) + ↓ +renderer.cc, renderer_sdf.cc, renderer_mesh.cc, renderer_skybox.cc + ↓ +ALL of 3d library rebuilds when asset_manager.h changes! +``` + +**Root Cause**: `object.h` unnecessarily includes full `asset_manager.h` for `AssetId mesh_asset_id`, but `AssetId` is already forward-declared! + +--- + +## Proposed Solution: Split asset_manager.h + +### New Structure + +#### File 1: `asset_manager_fwd.h` (Forward Declarations) +```cpp +// asset_manager_fwd.h - LIGHTWEIGHT, SAFE TO INCLUDE EVERYWHERE +#pragma once +#include <cstddef> +#include <cstdint> + +enum class AssetId : uint16_t; // Just the forward declaration + +typedef bool (*ProcGenFunc)(uint8_t*, int, int, const float*, int); + +struct AssetRecord; // Forward declaration (opaque) +``` + +#### File 2: `asset_manager.h` (Core API) +```cpp +// asset_manager.h - BASIC ASSET RETRIEVAL (most common use case) +#pragma once +#include "asset_manager_fwd.h" + +struct AssetRecord { + const uint8_t* data; + size_t size; + bool is_procedural; + const char* proc_func_name_str; + const float* proc_params; + int num_proc_params; +}; + +// Core API - most files only need this +const uint8_t* GetAsset(AssetId asset_id, size_t* out_size = nullptr); +void DropAsset(AssetId asset_id, const uint8_t* asset); +``` + +#### File 3: `asset_manager_helpers.h` (Typed Asset Helpers) +```cpp +// asset_manager_helpers.h - SPECIALIZED HELPERS (only 7 files need this) +#pragma once +#include "asset_manager.h" + +struct TextureAsset { + int width; + int height; + const uint8_t* pixels; +}; + +struct MeshVertex { + float p[3]; + float n[3]; + float u[2]; +}; + +struct MeshAsset { + uint32_t num_vertices; + const MeshVertex* vertices; + uint32_t num_indices; + const uint32_t* indices; +}; + +TextureAsset GetTextureAsset(AssetId asset_id); +MeshAsset GetMeshAsset(AssetId asset_id); +``` + +### Migration Plan + +#### Step 1: Fix object.h (QUICK WIN - 0 files) +```cpp +// object.h (BEFORE) +#include "util/asset_manager.h" // ❌ Pulls in everything + +// object.h (AFTER) +#include "util/asset_manager_fwd.h" // ✅ Only AssetId forward declaration +``` + +**Impact**: Changing `asset_manager.h` no longer rebuilds entire 3d library! + +#### Step 2: Update includes in all source files +```cpp +// Most files (10 files) - only need basic GetAsset +#include "util/asset_manager.h" + +// Specialized files (7 files) - need struct helpers +#include "util/asset_manager_helpers.h" +``` + +**Automated Migration**: +```bash +# Files using GetTextureAsset or GetMeshAsset +for file in renderer_mesh.cc visual_debug.cc flash_cube_effect.cc hybrid_3d_effect.cc; do + sed -i '' 's|asset_manager\.h|asset_manager_helpers.h|g' "src/**/$file" +done + +# All other files - keep as-is (will include base asset_manager.h) +``` + +--- + +## Expected Performance Improvements + +### Before Split +``` +Touch asset_manager.h → 35 files rebuild → 4.82s +``` + +### After Split (Estimated) +``` +Touch asset_manager_fwd.h → 0 files rebuild (forward decl only) → 0.2s ✅ +Touch asset_manager.h → 10 files rebuild → 1.5s ✅ +Touch asset_manager_helpers.h → 7 files rebuild → 1.0s ✅ +``` + +**Worst Case**: 1.5s (vs 4.82s) = **69% faster** ⚡ + +**Typical Case**: Most changes to asset_manager internals won't affect the API → 0.2s + +--- + +## Implementation Checklist + +### Phase 1: Create New Headers (30 minutes) +- [ ] Create `src/util/asset_manager_fwd.h` (forward declarations only) +- [ ] Split `src/util/asset_manager.h` → keep core API +- [ ] Create `src/util/asset_manager_helpers.h` → move TextureAsset/MeshAsset + +### Phase 2: Update Includes (30 minutes) +- [ ] Update `src/3d/object.h` → use `asset_manager_fwd.h` +- [ ] Update 7 files needing structs → use `asset_manager_helpers.h`: + - `src/3d/renderer_mesh.cc` + - `src/3d/visual_debug.cc` + - `src/3d/visual_debug.h` + - `src/gpu/effects/flash_cube_effect.cc` + - `src/gpu/effects/hybrid_3d_effect.cc` + - `src/tests/test_assets.cc` + - `src/tests/test_mesh.cc` +- [ ] Update `src/util/asset_manager.cc` → include `asset_manager_helpers.h` + +### Phase 3: Verify (15 minutes) +- [ ] Run all tests: `ctest` +- [ ] Measure rebuild time: `touch src/util/asset_manager.h && cmake --build build` +- [ ] Confirm <2s rebuild time + +**Total Effort**: ~75 minutes + +--- + +## Additional Low-Hanging Fruit + +### 1. Check other transitive includes +**Action**: Audit `object.h` for other unnecessary includes +```bash +# Find what object.h transitively pulls in +cpp -dM src/3d/object.h | wc -l +``` + +### 2. Use forward declarations in headers +**Pattern**: +```cpp +// BAD - pulls in entire header +#include "camera.h" + +// GOOD - forward declaration +class Camera; +``` + +### 3. Move inline functions to .cc files +**Problem**: Inline functions in headers force recompilation of all users when implementation changes. + +**Solution**: Move non-trivial inline functions to `.cc` files. + +--- + +## Risks & Mitigation + +### Risk 1: Breaking Existing Code +**Likelihood**: Low (forward declaration already exists) + +**Mitigation**: +- Compile after each file change +- Run full test suite before committing + +### Risk 2: Increased Include Complexity +**Likelihood**: Low (only 3 headers instead of 1) + +**Mitigation**: +- Clear naming: `_fwd.h` = forward decls, `_helpers.h` = specialized +- Document in CONTRIBUTING.md + +### Risk 3: Forgetting to Include Helpers +**Likelihood**: Medium (compiler errors if you use TextureAsset without including helpers) + +**Mitigation**: +- Compiler will error immediately with clear message +- Easy to fix: add `#include "asset_manager_helpers.h"` + +--- + +## Comparison with Original Proposal + +### Original Proposal 1 (Asset File Dependencies) +- **Impact**: Fixes correctness bug for asset changes +- **Performance**: No improvement (assets change rarely) +- **Recommendation**: LOW PRIORITY (assets rebuild is acceptable) + +### NEW Proposal (Split asset_manager.h) +- **Impact**: 69% faster rebuilds during core library iteration +- **Performance**: 4.82s → 1.5s (developer pain point!) +- **Recommendation**: HIGH PRIORITY ⭐ + +--- + +## Measurement Plan + +### Before Implementation +```bash +# Baseline: Current rebuild time +time (touch src/util/asset_manager.h && cmake --build build --target demo64k -j8) +# Expected: 4.82s, 35 files +``` + +### After Implementation +```bash +# Test 1: Forward declaration change (should be instant) +time (touch src/util/asset_manager_fwd.h && cmake --build build --target demo64k -j8) +# Expected: 0.2s, 0 files + +# Test 2: Core API change (should be fast) +time (touch src/util/asset_manager.h && cmake --build build --target demo64k -j8) +# Expected: 1.5s, 10 files + +# Test 3: Helper change (should be fast) +time (touch src/util/asset_manager_helpers.h && cmake --build build --target demo64k -j8) +# Expected: 1.0s, 7 files +``` + +--- + +## Other Headers to Investigate + +### Candidates for Similar Treatment +``` +mini_math.h → 1.9s rebuild (23 files) - already isolated to 3d library ✅ +camera.h → unknown (need measurement) +scene.h → unknown (need measurement) +``` + +**Recommendation**: Start with `asset_manager.h` (proven 4.8s bottleneck), then measure others if iteration is still slow. + +--- + +## Final Recommendation + +**Implement the header split immediately.** This is a high-impact, low-risk change that directly addresses the developer pain point (slow iteration during core library changes). + +**Do NOT implement asset file tracking** (Proposal 1 from original doc) - assets change infrequently, and 3.5s rebuild is acceptable. + +**Expected Result**: Developer iteration cycle improves from 4.8s → 1.5s (69% faster) for the most common workflow (tweaking core library code). |
