diff --git a/CHANGELOG.md b/CHANGELOG.md index f1464c3..e1c1a13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Windows x64. Importing `cel` leaves the GIL disabled ([#45](https://github.com/hardbyte/python-common-expression-language/issues/45)). +### Performance + +- **Parsing no longer holds the GIL.** `compile()`, and the parse inside + `evaluate()`, release the GIL for expressions of 32 bytes or more, so threads + that parse concurrently run in parallel: about 3× the throughput from 4 threads + on a 4-core machine, with single-threaded cost unchanged. Executing a compiled + program still holds the GIL + ([#45](https://github.com/hardbyte/python-common-expression-language/issues/45)). + ### Changed - `Program` and `OptionalValue` are immutable; attributes can no longer be diff --git a/docs/reference/python-api.md b/docs/reference/python-api.md index 0e70f2c..e93e552 100644 --- a/docs/reference/python-api.md +++ b/docs/reference/python-api.md @@ -41,6 +41,12 @@ assert result2 == 30 # → 30 - Use `evaluate()` for one-time evaluation or interactive/REPL usage - Use `compile()` + `execute()` when evaluating the same expression with many different contexts or in performance-critical loops +Parsing (`compile()`, and the parse inside `evaluate()`) runs with the GIL +released for expressions of 32 bytes or more, so threads that parse concurrently +run in parallel. Shorter expressions parse in a few microseconds, which is less +than the cost of re-acquiring a contended GIL, so they keep it. Executing a +compiled program also holds the GIL: evaluations are usually sub-microsecond. + ## Classes ### Program diff --git a/src/lib.rs b/src/lib.rs index b0c82c9..9566174 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -267,18 +267,37 @@ impl PyOptionalValue { /// >>> program.execute({"x": 10, "y": 20}) /// 30 #[pyfunction] -fn compile(expression: String) -> PyResult { - let program = compile_program(&expression)?; +fn compile(py: Python<'_>, expression: String) -> PyResult { + let program = compile_program(py, &expression)?; Ok(PyProgram { program, source: expression, }) } +/// Expressions shorter than this (in bytes) are parsed with the GIL held. +/// +/// A short parse costs about as much as re-acquiring a contended GIL, so +/// releasing it for one is a net loss under contention. Parse time grows with +/// expression length, which makes length a safe gate: erring long only forgoes a +/// speedup. Benchmarks behind the bound are in issue #45. +const PARSE_DETACH_MIN_LEN: usize = 32; + /// Parses `expression`, turning both parse errors and parser panics into /// `ValueError` so callers can rely on one exception type for a bad expression. -fn compile_program(expression: &str) -> PyResult { - panic::catch_unwind(|| Program::compile(expression)) +/// +/// The parse is pure Rust and cannot call back into Python, so it runs with the +/// GIL released unless the expression is too short for that to pay off. +/// `catch_unwind` sits inside the detached region so a parser panic is caught +/// before control crosses back through PyO3's re-attach guard. +fn compile_program(py: Python<'_>, expression: &str) -> PyResult { + let parse = || panic::catch_unwind(|| Program::compile(expression)); + let parsed = if expression.len() >= PARSE_DETACH_MIN_LEN { + py.detach(parse) + } else { + parse() + }; + parsed .map_err(|_| { warn!("CEL parser panic for expression: '{}'", expression); PyValueError::new_err(format!( @@ -1058,11 +1077,15 @@ impl TryIntoValue for RustyPyType<'_> { /// - CEL Language Guide: For comprehensive language documentation /// - Python API Reference: For detailed API documentation #[pyfunction(signature = (src, evaluation_context=None))] -fn evaluate(src: String, evaluation_context: Option<&Bound<'_, PyAny>>) -> PyResult { +fn evaluate( + py: Python<'_>, + src: String, + evaluation_context: Option<&Bound<'_, PyAny>>, +) -> PyResult { // Validate the context before parsing so a bad context and a bad expression // report in the same order they always have. let environment = prepare_environment(evaluation_context)?; - let program = compile_program(&src)?; + let program = compile_program(py, &src)?; run_program(&program, &src, &environment).map(RustyCelType) } diff --git a/tests/test_gil_release.py b/tests/test_gil_release.py new file mode 100644 index 0000000..65d08e1 --- /dev/null +++ b/tests/test_gil_release.py @@ -0,0 +1,104 @@ +"""Parsing releases the GIL. + +``cel.compile()`` and the parse inside ``cel.evaluate()`` run with the GIL +released for expressions of 32 bytes or more, so other Python threads make +progress while an expression is parsed. Shorter expressions parse in a few +microseconds, comparable to the cost of re-acquiring a contended GIL, so they keep +it. Execution of a compiled program still holds the GIL (see issue #45 for why: a +sub-microsecond evaluation loses badly to the cost of re-acquiring a contended +GIL, and a work-aware gate for execution is a separate change). + +The tests below detect release by *overlap* rather than by speed: a spinning +thread counts iterations while the main thread parses. With the GIL held for the +whole parse the spinner cannot run and the count stays at essentially zero; with +it released the spinner gets a core and the count climbs into the millions. That +is a binary signal, so it does not depend on the runner's core count or load. +""" + +import sys +import sysconfig +import threading + +import cel +import pytest + +# A list literal parses in time linear in its length; 20,000 elements is a few +# hundred milliseconds of pure parsing, long enough for the spinner to show up. +BIG_EXPRESSION = "[" + ", ".join(str(i) for i in range(20_000)) + "]" + +# On a free-threaded build the spinner runs regardless, so overlap proves nothing; +# the contract there is simply "still works", covered by the whole suite. +requires_gil_build = pytest.mark.skipif( + sys.implementation.name != "cpython" or bool(sysconfig.get_config_var("Py_GIL_DISABLED")), + reason="overlap is only observable on a CPython build with a GIL", +) + + +def ticks_during(action): + """Run ``action`` on this thread while another thread spins; return the spinner's count.""" + ticks = 0 + started = threading.Event() + stop = threading.Event() + + def spin(): + nonlocal ticks + started.set() + while not stop.is_set(): + ticks += 1 + + spinner = threading.Thread(target=spin) + spinner.start() + started.wait() + # Let the spinner settle so a switch-interval handoff right at the start does + # not count as overlap. + threading.Event().wait(0.02) + before = ticks + action() + after = ticks + stop.set() + spinner.join() + return after - before + + +@requires_gil_build +def test_compile_releases_the_gil(): + assert ticks_during(lambda: cel.compile(BIG_EXPRESSION)) > 10_000 + + +@requires_gil_build +def test_evaluate_releases_the_gil_while_parsing(): + assert ticks_during(lambda: cel.evaluate(BIG_EXPRESSION)) > 10_000 + + +def test_parse_results_are_unchanged(): + """Detaching changes where the parse runs, not what it produces or raises. + + Covers both sides of the length threshold: short expressions parse attached, + long ones detached, and errors surface identically from either path. + """ + assert cel.compile("1 + 2").execute() == 3 + assert cel.evaluate(BIG_EXPRESSION)[-1] == 19_999 + long_policy = 'user.role == "admin" || (resource.owner == user.id && size(user.groups) > 0)' + assert len(long_policy) >= 32 + assert cel.evaluate(long_policy, {"user": {"role": "admin"}, "resource": {}}) is True + with pytest.raises(ValueError, match="Failed to parse"): + cel.compile("1 +") + with pytest.raises(ValueError, match="Failed to parse"): + cel.evaluate("'unterminated") + with pytest.raises(ValueError, match="Failed to parse"): + cel.compile( + "this is a long enough expression to be parsed detached but it is not valid CEL !!" + ) + + +def test_concurrent_compiles_are_independent(): + """Many threads parsing at once each get their own correct program.""" + from concurrent.futures import ThreadPoolExecutor + + def work(i): + return cel.compile(f"{i} * 2 + 1").execute() + + with ThreadPoolExecutor(max_workers=8) as pool: + results = list(pool.map(work, range(400))) + + assert results == [i * 2 + 1 for i in range(400)]