Skip to content

Perf: read WKB coordinate runs in bulk, write WKT into one buffer #968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
giswqs merged 2 commits into from
Oct 26, 2026
Merged

Perf: read WKB coordinate runs in bulk, write WKT into one buffer #968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02b41dd
perf(data): read WKB coordinate runs in bulk, size the encoder buffer
Wolren Oct 26, 2026
9b8ee7d
perf(data): write WKT into one buffer, drop the per-coordinate Vec
Wolren Oct 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 85 additions & 19 deletions crates/data/src/interop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Writers for exchange formats: KML, GPX and CSV with WKT geometry.

use std::fmt::Write as _;

use rustgis_geom::Coord;

use crate::{Dataset, Geometry};
Expand Down Expand Up @@ -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 {

@github-actions github-actions Bot Oct 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let c = |p: &Coord| format!("{} {}", p.x, p.y);
let list = |v: &[Coord]| v.iter().map(c).collect::<Vec<_>>().join(", ");
fn coord(out: &mut String, p: &Coord) {
let _ = write!(out, "{} {}", p.x, p.y);
}
fn list(out: &mut String, v: &[Coord]) {

@github-actions github-actions Bot Oct 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (i, p) in v.iter().enumerate() {
if i > 0 {
out.push_str(", ");
}
coord(out, p);
}
}
fn rings(out: &mut String, rs: &[Vec<Coord>]) {
for (i, r) in rs.iter().enumerate() {
if i > 0 {
out.push_str(", ");
}
out.push('(');
list(out, r);
out.push(')');
}
}
let mut out = String::new();
match g {
Geometry::Point(p) => format!("POINT ({})", c(p)),
Geometry::MultiPoint(v) => format!("MULTIPOINT ({})", v.iter().map(|p| format!("({})", c(p))).collect::<Vec<_>>().join(", ")),
Geometry::Line(parts) if parts.len() == 1 => format!("LINESTRING ({})", parts.first().map(|p| list(p)).unwrap_or_default()),
Geometry::Point(p) => {
out.push_str("POINT (");
coord(&mut out, p);
out.push(')');
}
Geometry::MultiPoint(v) => {
out.push_str("MULTIPOINT (");
for (i, p) in v.iter().enumerate() {
if i > 0 {
out.push_str(", ");
}
out.push('(');
coord(&mut out, p);
out.push(')');
}
out.push(')');
}
Geometry::Line(parts) if parts.len() == 1 => {
out.push_str("LINESTRING (");
if let Some(p) = parts.first() {
list(&mut out, p);
}
out.push(')');
}
Geometry::Line(parts) => {
format!("MULTILINESTRING ({})", parts.iter().map(|p| format!("({})", list(p))).collect::<Vec<_>>().join(", "))
out.push_str("MULTILINESTRING (");
for (i, p) in parts.iter().enumerate() {
if i > 0 {
out.push_str(", ");
}
out.push('(');
list(&mut out, p);
out.push(')');
}
out.push(')');
}
Geometry::Polygon(polys) => {
let poly =
|rings: &Vec<Vec<Coord>>| format!("({})", rings.iter().map(|r| format!("({})", list(r))).collect::<Vec<_>>().join(", "));
if polys.len() == 1 {
format!("POLYGON {}", polys.first().map(poly).unwrap_or_default())
out.push_str("POLYGON (");
if let Some(rs) = polys.first() {
rings(&mut out, rs);
}
out.push(')');
} else {
format!("MULTIPOLYGON ({})", polys.iter().map(poly).collect::<Vec<_>>().join(", "))
out.push_str("MULTIPOLYGON (");
for (i, rs) in polys.iter().enumerate() {
if i > 0 {
out.push_str(", ");
}
out.push('(');
rings(&mut out, rs);
out.push(')');
}
out.push(')');
}
}
}
out
}

/// Decodes an encoded polyline (Google's algorithm; `precision` 5, or 6 as OSRM and Valhalla
Expand Down Expand Up @@ -291,23 +355,26 @@ impl WktParser {

/// `x y [z [m]]`.
fn coord(&mut self) -> Result<Coord, String> {
let mut v = Vec::with_capacity(4);
let mut nums = [0.0f64; 4];
let mut len = 0usize;
while let Some(WktTok::Num(n)) = self.peek() {
// Two to four numbers: a fifth is already wrong, so stop there.
if v.len() == 4 {
if len == nums.len() {
return Err("a coordinate needs two to four numbers".into());
}
v.push(*n);
nums[len] = *n;
len += 1;
Comment on lines +362 to +366

@github-actions github-actions Bot Oct 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

self.at += 1;
}
self.coords += 1;
if self.coords > MAX_WKT_COORDS {
return Err("too many coordinates".into());
}
match v.as_slice() {
[x, y, ..] if v.len() <= 4 && x.is_finite() && y.is_finite() => Ok(Coord::new(*x, *y)),
_ => Err("a coordinate needs two to four numbers".into()),
let (x, y) = (nums[0], nums[1]);
if len < 2 || !x.is_finite() || !y.is_finite() {
return Err("a coordinate needs two to four numbers".into());
}
Ok(Coord::new(x, y))
}

/// `( a, b, … )`, each item read by `item`.
Expand Down Expand Up @@ -351,12 +418,11 @@ impl WktParser {
if matches!(self.peek(), Some(WktTok::Word(w)) if matches!(w.as_str(), "Z" | "M" | "ZM")) {
self.at += 1;
}
let kind =
kind.strip_suffix("ZM").or_else(|| kind.strip_suffix('Z')).or_else(|| kind.strip_suffix('M')).unwrap_or(&kind).to_string();
let kind = kind.strip_suffix("ZM").or_else(|| kind.strip_suffix('Z')).or_else(|| kind.strip_suffix('M')).unwrap_or(&kind);
if self.empty() {
return Ok(None);
}
Ok(Some(match kind.as_str() {
Ok(Some(match kind {
"POINT" => {
self.expect(WktTok::Open)?;
let c = self.coord()?;
Expand Down
50 changes: 45 additions & 5 deletions crates/data/src/wkb.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub fn decode(bytes: &[u8]) -> Option<Geometry> {

/// Encodes a geometry as ISO WKB (2D, little-endian).
pub fn encode(g: &Geometry) -> Vec<u8> {
let mut out = Vec::new();
let mut out = Vec::with_capacity(wkb_size(g));
let head = |out: &mut Vec<u8>, ty: u32| {
out.push(1);
out.extend_from_slice(&ty.to_le_bytes());
Expand Down Expand Up @@ -66,6 +66,24 @@ pub fn encode(g: &Geometry) -> Vec<u8> {
out
}

/// Encoded width: 16 bytes per coordinate, 4 per count and 5 per geometry head.
fn wkb_size(g: &Geometry) -> usize {
let ring = |r: &[Coord]| 4 + 16 * r.len();
match g {
Geometry::Point(_) => 5 + 16,
Geometry::MultiPoint(v) => 5 + 4 + v.len() * (5 + 16),
Geometry::Line(parts) => 5 + 4 + parts.iter().map(|p| 5 + ring(p)).sum::<usize>(),
Geometry::Polygon(polys) => 5 + 4 + polys.iter().map(|p| 5 + 4 + p.iter().map(|r| ring(r)).sum::<usize>()).sum::<usize>(),
}
}

/// One field of a coordinate chunk; `None` when the chunk is too short.
#[inline]
fn f64_at(ch: &[u8], off: usize, little: bool) -> Option<f64> {
let a: [u8; 8] = ch.get(off..off + 8)?.try_into().ok()?;
Some(if little { f64::from_le_bytes(a) } else { f64::from_be_bytes(a) })
}

struct Reader<'a> {
b: &'a [u8],
pos: usize,
Expand Down Expand Up @@ -141,11 +159,16 @@ impl Reader<'_> {

/// A coordinate sequence, and whether every coordinate is finite. `None` when malformed.
fn coords(&mut self, h: Head) -> Option<(Vec<Coord>, bool)> {
let n = self.count(h, h.dims * 8)?;
let stride = h.dims * 8;
let n = self.count(h, stride)?;
let end = self.pos + n * stride;
let run = self.b.get(self.pos..end)?;
self.pos = end;
let mut v = Vec::with_capacity(n);
let mut finite = true;
for _ in 0..n {
let c = self.coord(h)?;
// The run is bounds-checked once; each chunk is exactly one coordinate.
for ch in run.chunks_exact(stride) {
let c = Coord::new(f64_at(ch, 0, h.little)?, f64_at(ch, 8, h.little)?);
finite &= c.is_finite();
v.push(c);
}
Expand Down Expand Up @@ -209,7 +232,14 @@ impl Reader<'_> {

/// Merges the members of a multi-geometry or collection into one geometry of the first member's
/// 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 {

@github-actions github-actions Bot Oct 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// A lone member needs no regrouping; a one-point multipoint still collapses.
return match parts.pop()? {
Geometry::MultiPoint(v) if v.len() == 1 => v.first().copied().map(Geometry::Point),
g => Some(g),
};
}
let first = parts.first()?.kind();
let mut points = Vec::new();
let mut lines = Vec::new();
Expand Down Expand Up @@ -246,8 +276,18 @@ mod tests {
Geometry::Point(Coord::new(1.5, -2.0)),
Geometry::MultiPoint(vec![Coord::new(1.0, 2.0), Coord::new(3.0, 4.0)]),
Geometry::Line(vec![vec![Coord::new(0.0, 0.0), Coord::new(1.0, 1.0)]]),
Geometry::Line(vec![vec![Coord::new(0.0, 0.0), Coord::new(1.0, 1.0)], vec![Coord::new(2.0, 2.0), Coord::new(3.0, 3.0)]]),
square(),
Geometry::Polygon(vec![vec![
vec![Coord::new(0.0, 0.0), Coord::new(9.0, 0.0), Coord::new(9.0, 9.0), Coord::new(0.0, 0.0)],
vec![Coord::new(1.0, 1.0), Coord::new(2.0, 1.0), Coord::new(2.0, 2.0), Coord::new(1.0, 1.0)],
]]),
Geometry::Polygon(vec![
vec![vec![Coord::new(0.0, 0.0), Coord::new(1.0, 0.0), Coord::new(1.0, 1.0), Coord::new(0.0, 0.0)]],
vec![vec![Coord::new(5.0, 5.0), Coord::new(6.0, 5.0), Coord::new(6.0, 6.0), Coord::new(5.0, 5.0)]],
]),
] {
assert_eq!(encode(&g).len(), wkb_size(&g));
assert_eq!(decode(&encode(&g)), Some(g));
}
}
Expand Down
Loading