perf: stop re-parsing the same GPIF document per track during a build - #52
Merged
Conversation
build_feedpak already loads and parses the whole GPIF (GP6/7/8) XML document once into gpif_root/gpif_tracks, but three downstream helpers were redundantly reloading and re-parsing it from disk: - _gpif_capo_lookup(gp_path) called gp2rs_gpx._load_gpif + _gpif_tracks again immediately after build_feedpak had just computed the same thing. - extract_gpif_sound_changes(gp_path, idx) reloaded the full document from disk on every iteration of the per-arrangement conversion loop (once per selected track), the single most expensive instance since it scales with track count. - _gpif_played_chord_names(root, track) rebuilt id-indexed lookup dicts for MasterBars/Bars/Voices/Beats/Notes from scratch on every call, even though those tables are identical for every track in the same file — split into a one-time _gpif_chord_context(root) builder plus a per-track lookup function that reuses it. All three functions keep working with just a gp_path/root=None for their existing standalone callers (tests, other pipeline stages); build_feedpak now passes through the document/tables it already has. Purely a hoist of invariant work out of a loop — no change to resolve-then-convert audio ordering, warp/offset semantics, or manifest shape. Verified with py_compile and the existing test suite (166 passed, 75 skipped — skips are pre-existing, host-fixture-gated). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer's GuideHoists GPIF XML parsing and chord-context construction out of per-track loops in build_feedpak by allowing helpers to accept pre-parsed documents, and reuses shared lookup tables across tracks to reduce redundant work without changing behavior. Sequence diagram for hoisted GPIF parsing and chord context reuse in build_feedpaksequenceDiagram
participant build_feedpak
participant gp2rs_gpx
participant _gpif_capo_lookup
participant _gpif_chord_context
participant _gpif_played_chord_names
participant tones_mod
build_feedpak->>gp2rs_gpx: _load_gpif(gp_path)
gp2rs_gpx-->>build_feedpak: gpif_root
build_feedpak->>gp2rs_gpx: _gpif_tracks(gpif_root)
gp2rs_gpx-->>build_feedpak: gpif_tracks
build_feedpak->>_gpif_capo_lookup: _gpif_capo_lookup(gp_path, gpif_root, gpif_tracks)
_gpif_capo_lookup-->>build_feedpak: gpif_capo
build_feedpak->>_gpif_chord_context: _gpif_chord_context(gpif_root)
_gpif_chord_context-->>build_feedpak: gpif_chord_ctx
loop per selected track idx
build_feedpak->>_gpif_played_chord_names: _gpif_played_chord_names(gpif_tracks[idx], gpif_chord_ctx)
_gpif_played_chord_names-->>build_feedpak: chord_names
build_feedpak->>tones_mod: extract_gpif_sound_changes(gp_path, idx, gpif_root)
tones_mod-->>build_feedpak: sound_changes
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider making the new helper parameters keyword-only (e.g.
def _gpif_capo_lookup(gp_path: str, *, root=None, raw_tracks=None)andextract_gpif_sound_changes(gp_path: str, track_index: int, *, root=None)) to avoid accidental positional misuse of the optional preloaded-document arguments. - It might be worth adding minimal type hints for the new
root,raw_tracks, andctxparameters/return values to keep their expected structure explicit and consistent with the rest of the module’s typing. - In
_gpif_chord_context, you could defensively assert or early-return ifrootisNone, to guard against accidental calls in non-GPIF contexts and make failures easier to diagnose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making the new helper parameters keyword-only (e.g. `def _gpif_capo_lookup(gp_path: str, *, root=None, raw_tracks=None)` and `extract_gpif_sound_changes(gp_path: str, track_index: int, *, root=None)`) to avoid accidental positional misuse of the optional preloaded-document arguments.
- It might be worth adding minimal type hints for the new `root`, `raw_tracks`, and `ctx` parameters/return values to keep their expected structure explicit and consistent with the rest of the module’s typing.
- In `_gpif_chord_context`, you could defensively assert or early-return if `root` is `None`, to guard against accidental calls in non-GPIF contexts and make failures easier to diagnose.
## Individual Comments
### Comment 1
<location path="feedpakr_tones.py" line_range="151-154" />
<code_context>
-def extract_gpif_sound_changes(gp_path: str, track_index: int) -> dict | None:
+def extract_gpif_sound_changes(gp_path: str, track_index: int, root=None) -> dict | None:
"""GPIF-only — reads a *different* mechanism than parse_tones_xml: a
track-level `<Automations><Automation><Type>Sound</Type>…` list, which
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid re-running `_gpif_tracks(root)` on every track when a shared `root` is provided.
With the new `root` parameter, this still calls `_gpif_tracks(root)` on every invocation, so `build_feedpak` re-walks the GPIF document once per track. Consider mirroring `_gpif_capo_lookup` by accepting an optional `raw_tracks` (or precomputed track map) so callers can reuse the traversal across tracks.
Suggested implementation:
```python
return times
def extract_gpif_sound_changes(
gp_path: str,
track_index: int,
root=None,
raw_tracks=None,
) -> dict | None:
"""GPIF-only — reads a *different* mechanism than parse_tones_xml: a
track-level `<Automations><Automation><Type>Sound</Type>…` list, which
swaps the MIDI/RSE instrument a track plays at a given bar (e.g. a keys
extract_gp345_tones (feedpak spec §6.9) — no `rig`/`base_rig`, since
GP's RSE softsynth patches aren't portable rig data (no .sf2 ships with
the source); a Reader gets the tone-change *names* honestly rather than
an invented playable rig.
``root``, when given, is an already-parsed GPIF document — the caller
(build_feedpak) invokes this once per track in its conversion loop, so
passing the root it already loaded avoids re-parsing the same GPIF file.
``raw_tracks``, when given, is a precomputed track map (typically the
result of ``_gpif_tracks(root)``). Supplying this lets callers reuse a
single traversal of the GPIF document across tracks instead of calling
``_gpif_tracks`` on every invocation.
```
Inside `extract_gpif_sound_changes`, wherever `_gpif_tracks(root)` is currently being called, update the implementation to reuse a shared `raw_tracks` when supplied, and only compute it when necessary. For example, near the top of the function body:
```python
if raw_tracks is None:
if root is None:
# mirror the pattern used elsewhere (e.g. in _gpif_capo_lookup):
root = _gpif_root(gp_path)
raw_tracks = _gpif_tracks(root)
```
Then, replace any usage of `_gpif_tracks(root)` in the function with `raw_tracks`. Finally, update `build_feedpak` (and any other callers) to:
1. Parse the GPIF root once.
2. Call `_gpif_tracks(root)` once to obtain `raw_tracks`.
3. Pass both `root` and `raw_tracks` into `extract_gpif_sound_changes` for each track, so that the GPIF traversal is reused rather than repeated per track.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ℹ️ One remaining per-track redundancy in the build loop; minor suggestions inline.
Reviewed changes
_gpif_capo_lookupparameterized — accepts optionalroot/raw_tracksto skip re-parsing;build_feedpakthreads its already-loaded values through. Standalone callers unchanged. Clean._gpif_played_chord_namessplit into_gpif_chord_context+ per-track lookup — the five document-wide id-indexed dicts (MasterBars, Bars, Voices, Beats, Notes) are now built once per GPIF file instead of once per track. Correct O(tracks × doc) → O(doc + tracks) improvement. The function's signature changed from(root, track)to(track, ctx), but no standalone callers exist in the codebase, so the break is safe.extract_gpif_sound_changesaccepts optionalroot—build_feedpakpasses the already-loaded document. Standalone callers unaffected.- Version bump to
0.7.2— appropriate for a user-visible perf improvement.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏
…track loop Sourcery review on #52 caught that extract_gpif_sound_changes still called _gpif_tracks(root) on every track invocation even with a shared root passed in, undercutting the PR's own goal of not re-walking the GPIF document per track. Thread the already-computed gpif_tracks through the same way _gpif_capo_lookup already does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
build_feedpakalready loads/parses the full GPIF (GP6/7/8) XML document once intogpif_root/gpif_tracks, but three downstream helpers were redundantly re-loading and re-parsing that same document from scratch:_gpif_capo_lookup(gp_path)— reloaded it once more right afterbuild_feedpakhad just computed the same thing.extract_gpif_sound_changes(gp_path, idx)— reloaded the entire document from disk on every iteration of the per-arrangement conversion loop (once per selected GPIF track). The most expensive of the three, since it scales with track count._gpif_played_chord_names(root, track)— rebuilt id-indexed lookup dicts (MasterBars/Bars/Voices/Beats/Notes) from scratch on every call, even though those tables are identical for every track in the same file.All three now accept an optional pre-loaded document, defaulting to the old load-from-path behavior for their existing standalone callers (e.g.
tests/test_pipeline.py::test_gpif_capo_lookup_reads_capo_fret)._gpif_played_chord_namesis split into a one-time_gpif_chord_context(root)builder plus a per-track lookup that reuses those tables — O(tracks × document size) → O(document size + tracks) for chord-name extraction.build_feedpaknow threads its already-loadedgpif_root/gpif_tracksthrough to all three call sites.This is a pure hoist of invariant work out of a loop. The resolve-then-convert audio ordering, warp/offset semantics, and manifest shape (all documented as previously-fixed subtle bugs in CLAUDE.md) are untouched.
Type of Change
Testing
python3 -m py_compile feedpakr_pipeline.py feedpakr_tones.py— cleanRepository Relevance
Checklist
Generated by Claude Code
Summary by Sourcery
Eliminate redundant GPIF parsing and lookup construction during multi-track feed builds without changing output behavior.
Enhancements:
Tests: