Skip nil input and memoize plconvert#55
Open
tilthouse wants to merge 1 commit intoajrosen:mainfrom
Open
Conversation
`plconvert` is called twice per reminder by `Reminder#initialize`
(once for `color`, once for `messaging`). Each call forks
`/usr/bin/plutil`. On a 4000-reminder store that's ~8000 fork+exec
invocations, dominating cold-start runtime for `tasks`.
Two observations make the work redundant:
- `messaging` (zContactHandles) is null for the vast majority of
reminders in real-world data — every `plconvert(nil)` still forks
plutil only to come back with nothing.
- `color` is a list-level attribute that the SQL JOIN copies onto
every reminder row. So the same ~12 unique color blobs are
re-parsed thousands of times.
Fix:
- Early-return `nil` for nil/empty input, skipping the fork.
- Memoize results by input bytes in a process-scoped hash. nil
results are cached too so failing blobs aren't retried per row.
Behavior is preserved: parsed objects for the same input are
byte-identical (we cache plutil's own output), and nil inputs already
went through the rescue-or-return-nil path implicitly.
## Tests
Adds `test/plconvert_test.rb` — 5 cases using stdlib minitest:
- nil input returns nil without spawning a subprocess
- empty string returns nil without spawning a subprocess
- repeated calls with the same input only fork plutil once
- distinct inputs each spawn their own subprocess
- a blob that produces nil is still cached (no re-fork)
The Open3 stub is implemented via singleton-method aliasing rather
than `Minitest::Mock` because the latter was removed in minitest 6.x.
Run with:
ruby test/plconvert_test.rb
## Notes
- No changes to runtime dependencies, gemspec, or executable surface.
- No public API changes — `plconvert` signature and return shape are
unchanged.
- Process-scoped cache; not thread-safe (icalPal is single-threaded).
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
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
plconvert(inlib/utils.rb) is called twice per reminder byReminder#initialize— once forcolor, once formessaging. Each call forks/usr/bin/plutil. On a typical 4000-reminder store that's ~8000 fork+exec invocations, which dominates cold-start runtime for thetaskscommand.Two observations on real data make the work redundant:
messaging(zContactHandles) is null for the vast majority of reminders. Everyplconvert(nil)call still forks plutil, sends an empty stdin, and comes back with nothing.coloris a list-level attribute that the SQL JOIN duplicates onto every reminder row. ~12 unique color blobs across ~40 lists end up getting re-parsed thousands of times.Empirical impact (3853-reminder store):
plutilinvocations: ~0.43 s.This is one of two upstream fixes that take this user's
icalpal taskscold start from ~100 s to ~0.6 s. The other is filed separately as an issue (theZMembershipsOfRemindersInSectionsAsDatablob join inReminder::QUERY— happy to file that as a PR too once we agree on the right SQL approach).The fix
Two changes in
lib/utils.rb:Plus a top-level
PLCONVERT_CACHE = {}constant for the cache itself.The
cached || PLCONVERT_CACHE.key?(obj)check makes nil results cacheable too — a blob that fails to parse (Plist::UnimplementedElementErrorreturns nil) doesn't get re-tried per row.Behavior preservation
Tests
Adds
test/plconvert_test.rb— 5 cases using stdlib minitest:The Open3 stub is implemented via singleton-method aliasing rather than
Minitest::Mockbecause the latter was removed in minitest 6.x — the test file should run cleanly on either minitest version.Run with:
I confirmed the suite catches the regression — reverting the patch causes all 5 tests to fail.
Notes
PLCONVERT_CACHEcould be private inside a module if you'd prefer), wrap the cache in something LRU-bounded, or split the changes if you want them in smaller pieces.