Prune placeholders only when a null was inlined - #153
Merged
Conversation
1.15.1 rewrites `field == ?` bound to null into `field IS NULL`, which
leaves that parameter unreferenced, and found it by materializing the
encoded output and scanning it for every placeholder. Every query paid
that, including the overwhelmingly common one comparing nothing against
null: ~0.30us per encoded condition (3-placeholder condition), against the
~0.90us of a whole logged route call after the 1.15.0 dispatch work.
Only a placeholder actually rewritten to `IS NULL`/`IS NOT NULL` can become
unreferenced, so `EncodingContext` now records those keys as they are
written, and the prune returns immediately when the set is empty — no
`toString()`, no scan. The output is materialized lazily even when it is
non-empty, since a recorded key may still be referenced by another operator
(`field > ?` bound to null), and then there is nothing to look for.
The set is left null while empty: allocating one per encoded condition
would give back part of what this saves.
No behaviour change: same statements, same bound parameters. Verified by
the suites that caught the original bug — PostgreSQL 65/65 and MySQL 65/65,
where an unreferenced parameter is a hard error ("Contains superfluous
variables").
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #153 +/- ##
==========================================
- Coverage 68.24% 68.24% -0.01%
==========================================
Files 66 66
Lines 22147 22160 +13
==========================================
+ Hits 15114 15122 +8
- Misses 7033 7038 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Follow-up to #152, which introduced a cost this removes.
The regression
1.15.1 rewrites
field == ?bound to null intofield IS NULL. That leaves the parameter resolved but unreferenced, and PostgreSQL rejects a statement carrying variables it does not use — so #152 added a prune that materialized the encoded output and scanned it for every placeholder:Every query paid it, including the overwhelmingly common one that compares nothing against null — O(placeholders × output length), plus a
'@$key'allocation per placeholder, plus the fulltoString().Measured in isolation on a 3-placeholder condition of the shape entity queries produce:
For scale, the 1.15.0 dispatch work brought a whole logged route call to ~0.90us. This was adding roughly a third of that back, on every query.
The fix
Only a placeholder actually rewritten to
IS NULL/IS NOT NULLcan become unreferenced.EncodingContextnow records those keys as they are written, so:toString(), no scan, no allocation;field > ?bound to null) and then there is nothing to look for.The set is left null while empty — allocating one per encoded condition would give back part of the saving.
Verification
No behaviour change is intended: same statements, same bound parameters. Checked against the suites that caught the original bug, where an unreferenced parameter is a hard error (
Contains superfluous variables):ensure_buildincludeddart analyze --fatal-infos --fatal-warningsanddart format --set-exit-if-changedclean🤖 Generated with Claude Code