perf: limit memory allocation during Vector deserialization#759
perf: limit memory allocation during Vector deserialization#759
Conversation
ivokub
added
type: bug
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes vector deserialization to prevent out-of-memory (OOM) issues when handling tampered or malicious input with excessively large headers. The implementation introduces batched memory allocation (up to 4GB chunks) instead of allocating the entire vector upfront, and reuses existing slice capacity when available.
Key changes:
- Modified
ReadFromandAsyncReadFromto allocate memory in smaller batches - Added
Vector.Equalmethod for vector equality checking - Enhanced error handling with more descriptive messages
Reviewed Changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| field/generator/internal/templates/element/vector.go | Template file implementing batched allocation logic and Equal method |
| field/generator/internal/templates/element/tests_vector.go | Template for test cases validating new deserialization behavior |
| field/*/vector.go | Generated implementation files from template |
| field/*/vector_test.go | Generated test files validating error handling and capacity reuse |
Comments suppressed due to low confidence (7)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
gbotrel
left a comment
gbotrel
left a comment
There was a problem hiding this comment.
lgtm, couldn't find any issue 👍
Bug: Incorrect EOF Handling in ReadFrom MethodThe Additional Locations (1) |
Description
NB! Should be backported to 0.18.0 and 0.19.0.
We previously allocated the whole
Vector(internally[]Element) after reading the number of elements in the serialized header. However, as we read the input from a reader we actually don't know if the input really holds that much elements. This PR allocates smaller (up to 4GB) batches from memory.Additionally, we now avoid allocating memory in case the
Vectorcapacity is large enough to store the elements.There is performance regression in case of large inputs when we have to allocate several times but this can be mitigated by preallocating the slices.
Added
Vector.Equalmethod for vector equality check.Benchmarking
fr_bn254.Vectorimplementation between old and new:Different choices for the allocation:
Type of change
Please delete options that are not relevant.
How has this been tested?
How has this been benchmarked?
Checklist:
golangci-lintdoes not output errors locallyNote
Refactors Vector deserialization to read in bounded chunks and reuse capacity, adds Vector.Equal, improves errors/docs, and introduces targeted tests and benchmarks across all fields/curves.
Vector.Equalviaslices.Equal.Vector.Equal.ReadFromandAsyncReadFrom(prealloc vs empty).Written by Cursor Bugbot for commit 44fa25c. This will update automatically on new commits. Configure here.