Repository navigation
Perf: read WKB coordinate runs in bulk, write WKT into one buffer - #968
Conversation
The coordinate loop checked the buffer bound and advanced the cursor once per value. Reader::coords now bounds-checks the whole run once and reads fixed offsets inside it, so a 2D coordinate costs two loads instead of a cursor move per field. Z and M values are still consumed through the stride; only X and Y are stored. A multi-geometry or collection with a single member is returned directly instead of being regrouped into a fresh geometry. A one-point multipoint still collapses to a point, as before. encode now sizes its buffer with a private wkb_size: 16 bytes per coordinate, 4 per count and 5 per geometry head. The round-trip test covers a two-part line, a polygon with a hole and a two-member multipolygon.
The writer built a String per coordinate and per ring, then joined them with collect. It appends into a single buffer with fmt::Write instead; the output is byte-identical on a 1.58M-polygon corpus. WktParser::coord kept each coordinate's numbers in a Vec; it now uses a fixed 4-element array, and the geometry type word is no longer copied into a second String. Accepted inputs and error messages are unchanged.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
WKT formatting and parsing crates/data/src/interop.rs |
WKT output is written into one reusable buffer. Coordinate parsing uses a four-value array while retaining its existing validation limits. Geometry type matching uses a borrowed string. |
WKB sizing, decoding, and merging crates/data/src/wkb.rs |
Encoding reserves capacity using wkb_size. Coordinate decoding checks and reads the full coordinate run in fixed-width chunks. A single geometry is returned directly, except a one-point MultiPoint becomes a Point. Round-trip tests cover additional multipart and polygon cases and check encoded lengths. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Refactor
Suggested reviewers: giswqs
Merge Risk: 
9b8ee
-
No actionable merge-blocking issue is established; the PR is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly summarizes the two main performance changes: bulk WKB coordinate reads and single-buffer WKT writing. Description check The description includes the required Summary, Testing, and Related issue sections. It explains the motivation, implementation, compatibility guarantees, benchmark results, test results, and known env… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description includes the required Summary, Testing, and Related issue sections. It explains the motivation, implementation, compatibility guarantees, benchmark results, test results, and known environment-specific test failure.
-
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit watches WKT flow,
One buffer holds each shape in line.
WKB counts its bytes before,
Then reads coordinates in their time.
A lone small point hops out as Point,
While holes and parts join tests in rhyme.
Comment @coderabbitai help to get the list of available commands.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if len == nums.len() { | ||
| return Err("a coordinate needs two to four numbers".into()); | ||
| } | ||
| v.push(*n); | ||
| nums[len] = *n; | ||
| len += 1; |
There was a problem hiding this comment.
AGENTS.md (nit): nums[len] indexes on a value parsed from the input. It cannot panic here because of the length check above it, but AGENTS.md asks for get() over indexing on hostile input. The get_mut form folds the bound check into the write:
| if len == nums.len() { | |
| return Err("a coordinate needs two to four numbers".into()); | |
| } | |
| v.push(*n); | |
| nums[len] = *n; | |
| len += 1; | |
| let Some(slot) = nums.get_mut(len) else { | |
| return Err("a coordinate needs two to four numbers".into()); | |
| }; | |
| *slot = *n; | |
| len += 1; |
Confidence: high that this is behaviourally identical; low severity.
| @@ -116,25 +118,87 @@ pub fn to_gpx(ds: &Dataset, name_field: Option<usize>) -> String { | |||
|
|
|||
| /// Well-known text for a geometry. | |||
| pub fn wkt(g: &Geometry) -> String { | |||
There was a problem hiding this comment.
Test coverage (medium): The wkt() rewrite touches every branch (MULTIPOINT, MULTILINESTRING, POLYGON with holes, MULTIPOLYGON), but the only direct assertions in writes_kml_gpx_wkt are POINT and LINESTRING. The byte-identity claim rests on the author's external corpus run, which CI does not repeat. Suggest a small test that pins the exact text for each variant, plus a parse_wkt(&wkt(g)) round trip for the multi-part ones, so a future regression in the separator or parenthesis logic fails here.
| /// kind; members of other kinds are dropped. | ||
| fn merge(parts: Vec<Geometry>) -> Option<Geometry> { | ||
| fn merge(mut parts: Vec<Geometry>) -> Option<Geometry> { | ||
| if parts.len() == 1 { |
There was a problem hiding this comment.
Test coverage (medium): The new single-member fast path changes behaviour in one case that no test exercises: a one-point MultiPoint decodes to Point, and a single-member collection decodes to its member. The existing round_trips test never builds either input. I checked by reading that the fast path matches the old merge result for Point, MultiPoint (0, 1 and many points), Line and Polygon, so the code looks right. Adding a decode assertion for the one-point case would lock that in.
| fn coord(out: &mut String, p: &Coord) { | ||
| let _ = write!(out, "{} {}", p.x, p.y); | ||
| } | ||
| fn list(out: &mut String, v: &[Coord]) { |
There was a problem hiding this comment.
Quality (nit): wkt() now hand-rolls the same if i > 0 { push ", " } loop five times (list, rings, MULTIPOINT, MULTILINESTRING, MULTIPOLYGON). One generic helper would remove most of the 60 added lines and keep the separator rule in one place:
fn join<T>(out: &mut String, items: &[T], mut each: impl FnMut(&mut String, &T)) {
for (i, item) in items.iter().enumerate() {
if i > 0 {
out.push_str(", ");
}
each(out, item);
}
}Then list becomes join(out, v, coord), and rings becomes join(out, rs, |out, r| { out.push('('); list(out, r); out.push(')'); }). Output stays the same.
Code reviewI read the full diff against the surrounding code and checked the behaviour by reading. I did not compile or run the tests. The output is unchanged in every case I checked: the WKB encoder and decoder, the WKT writer and the WKT parser's Bugs
Security
Performance
Quality
AGENTS.md
Test coverage
Inline comments were posted on Change sizeCompared: the whole PR against main (merge base 9285e05 to PR head 9b8ee7d).
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
|
Great improvement. Thanks. |
Summary
rustgis-datareads a WKB coordinate run one value at a time and writes WKT through aStringper coordinate, joined at the end. On real polygon data both paths spend most of their time in that per-value work: a bounds check and a cursor move on every field, an allocation on every coordinate.This reads a whole coordinate run once, takes fixed offsets inside it, sizes the WKB buffer exactly instead of letting it grow, and appends WKT into a single buffer.
WktParser::coordno longer allocates aVecper coordinate, and the geometry type word is no longer copied into a secondString.No new dependency, no
unsafe, no public API change, no command or format change. The encoded bytes and the WKT text are identical to the current output.Testing
The corpus is the 813 IUCN Red List species range multipolygons (755 species, EPSG:4326), 782 MB of WKB taken from GeoPackage geometry blobs. 752 decode; the other 61 are empty multipolygons, which the reader returns as
None. That is 1,579,030 polygon members and 47.6M vertices.Nanoseconds per polygon member, release build on Windows 11 with rustc 1.98.1, min of six passes, one build of each revision, the two builds measured back to back:
Repeat runs of the unmodified build spread by 5 to 27 percent per operation. The WKB decode row is the widest, and its gain here sits inside that spread, so it is not claimed. A 20-pass run of decode alone puts the medians at 341 ns (main) and 312 ns (this branch).
0x813a3f89502102f2in both builds, and over the WKT text (1,828,808,256 bytes)0xc9da3afb960e68b8in both. The decoded coordinates hash the same in both.cargo test -p rustgis-data --lib: 379 passed, 0 failed, 10 ignored, with the GDAL-dependent FileGDB test filtered out.round_tripsnow also covers a two-part line, a polygon with a hole and a two-member multipolygon, and checks that the encoded length equals the sizeencodereserves.cargo fmt --all --checkandcargo clippy --workspace --all-targets -- -D warningspass on the branch. The one test that fails locally,filegdb::tests::reads_what_gdal_writes_when_gdal_is_installed, fails the same way onmain: the local GDAL 3.9.2 refuses a TIN in an OpenFileGDB (Can only insert a Polygon/MultiPolygon/CurvePolygon/MultiSurface in a esriGeometryPolygon layer).Related issue
None. This changes the speed of existing codecs, not their behaviour.
Summary by CodeRabbit