You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
This PR ensures that we allocate exactly the amount of memory needed for Vec when we deserialize a JSON array (assuming that every element is deserialized).
This makes sure that converting a Vec into a boxed slice doesn't re-allcoate.
I had to add impl ExactSizeIterator for iterators in biome_rowan.
AstSeparatedListElementsIterator can implement ExactSizeIterator because it builds on SyntaxSlots that implements ExactSizeIterator and no elements from SyntaxSlots is skipped.
AstSeparatedListNodesIterator can implement ExactSizeIterator because it builds on AstSeparatedListElementsIterator that implements ExactSizeIterator and it just maps iterated elements in its Ietartor::next function.
Note: we could certainly improve the implementation of Ietrator for AstSeparatedListElementsIterator and AstSeparatedListNodesIterator by implementing methods like last, ... This is out of scope of this PR.
I changed the signature of Deserializable::visit_array and Deserializable::visit_map to accept ExactSizeIterator instead of Iterator.
This is a backward compatible change because an implementor can still use impl Iterator (we have a few places where it is still the case).
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Trait methods visit_array and visit_map in crates/biome_deserialize now accept ExactSizeIterator instead of Iterator. Implementations for Vec, smallvec::SmallVec, HashSet, BTreeSet, indexmap::IndexSet, HashMap, BTreeMap and indexmap::IndexMap were updated to match, with several constructors now pre‑allocating and extending. In crates/biome_rowan, two AST iterators gained precise size_hint implementations and ExactSizeIterator conformance.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%.
You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name
Status
Explanation
Title check
✅ Passed
The title accurately summarises the main change: refactoring deserialization to use exact allocation for arrays and maps via ExactSizeIterator.
Description check
✅ Passed
The description is comprehensive and directly related to the changeset, explaining the motivation, implementation details, and rationale for API signature changes.
✨ Finishing touches
📝 Docstrings were successfully generated.
🧪 Generate unit tests (beta)
Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch conaclos/deserialize-exact-allocation
Comment @coderabbitai help to get the list of available commands and usage tips.
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
This PR ensures that we allocate exactly the amount of memory needed for
Vecwhen we deserialize a JSON array (assuming that every element is deserialized).This makes sure that converting a
Vecinto a boxed slice doesn't re-allcoate.I had to add
impl ExactSizeIteratorfor iterators inbiome_rowan.AstSeparatedListElementsIteratorcan implementExactSizeIteratorbecause it builds onSyntaxSlotsthat implementsExactSizeIteratorand no elements fromSyntaxSlotsis skipped.AstSeparatedListNodesIteratorcan implementExactSizeIteratorbecause it builds onAstSeparatedListElementsIteratorthat implementsExactSizeIteratorand it just maps iterated elements in itsIetartor::nextfunction.Note: we could certainly improve the implementation of
IetratorforAstSeparatedListElementsIteratorandAstSeparatedListNodesIteratorby implementing methods likelast, ... This is out of scope of this PR.I changed the signature of
Deserializable::visit_arrayandDeserializable::visit_mapto acceptExactSizeIteratorinstead ofIterator.This is a backward compatible change because an implementor can still use
impl Iterator(we have a few places where it is still the case).Test Plan
CI should still be green.
Docs
No change.