Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 5 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ fn extract_multipolygon(coordinates: &Bound<'_, PyAny>) -> PyResult<Vec<Polygon<

#[pyfunction]
fn polygon_to_geohashes(
_py: Python,
py: Python<'_>,
py_polygon: Bound<'_, PyAny>,
precision: usize,
inner: bool,
Expand Down Expand Up @@ -569,7 +569,10 @@ fn polygon_to_geohashes(
}
};

polygons_to_geohashes(polygons, precision, inner)
// The cover walk touches no Python objects, and at fine precisions it runs for
// seconds — verdun at precision 10 takes minutes. Hold the GIL for the
// __geo_interface__ read above only, not for the geometry.
py.detach(|| polygons_to_geohashes(polygons, precision, inner))
.map_err(|e| pyo3::exceptions::PyValueError::new_err(format!("{e:?}")))
}

Expand Down
60 changes: 60 additions & 0 deletions tests/test_geohasher.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os
import shapely
import geohash_polygon
from polygon_geohasher.polygon_geohasher import (
Expand Down Expand Up @@ -249,3 +250,62 @@ def test_hole(level, inner, polygon_hole):
assert geohash_polygon.polygon_to_geohashes(
polygon_hole, level, inner
) == polygon_to_geohashes_py(polygon_hole, level, inner)


@pytest.mark.skipif(
(os.cpu_count() or 1) < 2,
reason="two CPU-bound threads cannot overlap on a single core, GIL or not",
)
def test_polygon_to_geohashes_releases_the_gil(polygon_verdun):
"""The cover walk must not hold the GIL.

Two concurrent calls should overlap rather than serialise. If the GIL were
held for the whole walk, the pair would take about twice a single call.
"""
import threading
import time

precision = 9
# One call is short enough that thread start-up and scheduler noise move the
# ratio by more than the effect being measured. Repeating inside each timing
# makes the walk itself dominate.
repeats = 10

def cover():
for _ in range(repeats):
geohash_polygon.polygon_to_geohashes(polygon_verdun, precision, False)

# Warm up so neither timing pays one-off costs.
cover()

start = time.perf_counter()
cover()
single = time.perf_counter() - start
Comment on lines +268 to +283

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Precision drops to 8 later in the stack (#40), for a different reason — above PARALLEL_COVER_MIN_CELLS one call already saturates every core and the test cannot tell a held GIL from a released one. The runtime concern lands in the same place. Full pytest run on this branch is ~10s.


# Threads start one at a time, so without a gate the first would have a head
# start on the second and the pair would look more overlapped than it is.
gate = threading.Barrier(3)
failures = []

def work():
gate.wait()
try:
cover()
except Exception as exc: # threading swallows these otherwise
failures.append(exc)

threads = [threading.Thread(target=work) for _ in range(2)]
for t in threads:
t.start()
gate.wait()
start = time.perf_counter()
for t in threads:
t.join()
concurrent = time.perf_counter() - start
assert not failures, f"worker thread raised: {failures[0]!r}"

# Serialised would be ~2.0x. Allow generous headroom for a loaded machine.
assert concurrent < single * 1.7, (
f"two concurrent calls took {concurrent:.3f}s against {single:.3f}s for one "
f"({concurrent / single:.2f}x) — the GIL looks held for the duration"
)
Loading