diff --git a/csrc/faster_eval_api/coco_eval/dataset.cpp b/csrc/faster_eval_api/coco_eval/dataset.cpp index 19d27e8..2eb877e 100644 --- a/csrc/faster_eval_api/coco_eval/dataset.cpp +++ b/csrc/faster_eval_api/coco_eval/dataset.cpp @@ -3,6 +3,7 @@ #include #include #include +#include // clang-format off #include "cocoeval.h" @@ -119,6 +120,119 @@ std::vector LightweightDataset::get(double img_id, double cat_id) { } } +namespace { + +// Annotation field names, interned once for the process lifetime. +// +// Passing a C string literal to dict::contains or operator[] makes pybind11 +// build a fresh Python str for every lookup: allocate, decode UTF-8, hash with +// siphash24 because a new str has no cached hash, then free. That repeated for +// seven fields on every annotation dominated annotation parsing. An interned +// str is built once and carries its hash, so lookups become a plain +// PyDict_GetItem probe. +struct AnnotationKeys { + PyObject* id; + PyObject* score; + PyObject* area; + PyObject* is_crowd; + PyObject* iscrowd; + PyObject* ignore; + PyObject* lvis_mark; + + AnnotationKeys() + : id(PyUnicode_InternFromString("id")), + score(PyUnicode_InternFromString("score")), + area(PyUnicode_InternFromString("area")), + is_crowd(PyUnicode_InternFromString("is_crowd")), + iscrowd(PyUnicode_InternFromString("iscrowd")), + ignore(PyUnicode_InternFromString("ignore")), + lvis_mark(PyUnicode_InternFromString("lvis_mark")) {} + + // Owns seven raw PyObject* and defines a destructor: forbid copy and + // move so an accidental by-value use cannot double Py_XDECREF the + // interned strings. The single instance is reached via + // annotation_keys(). + AnnotationKeys(const AnnotationKeys&) = delete; + AnnotationKeys& operator=(const AnnotationKeys&) = delete; + AnnotationKeys(AnnotationKeys&&) = delete; + AnnotationKeys& operator=(AnnotationKeys&&) = delete; + + bool complete() const { + return id != nullptr && score != nullptr && area != nullptr && + is_crowd != nullptr && iscrowd != nullptr && + ignore != nullptr && lvis_mark != nullptr; + } + + ~AnnotationKeys() { + Py_XDECREF(id); + Py_XDECREF(score); + Py_XDECREF(area); + Py_XDECREF(is_crowd); + Py_XDECREF(iscrowd); + Py_XDECREF(ignore); + Py_XDECREF(lvis_mark); + } +}; + +// A copy or move would run ~AnnotationKeys twice on the same interned +// PyObject*, double Py_XDECREF them and corrupt interpreter refcounts. +static_assert(!std::is_copy_constructible::value && + !std::is_move_constructible::value && + !std::is_copy_assignable::value && + !std::is_move_assignable::value, + "AnnotationKeys must not be copyable or movable"); + +// Deliberately leaked: interpreter teardown may run static destructors without +// the GIL, and releasing Python objects there is undefined behaviour. The keys +// are needed for as long as the module is usable, so never freeing them costs +// seven small strings and removes the shutdown hazard. +const AnnotationKeys& annotation_keys() { + static const AnnotationKeys* keys = [] { + auto* candidate = new AnnotationKeys(); + if (candidate->complete()) { + return candidate; + } + + delete candidate; + if (PyErr_Occurred()) { + throw py::error_already_set(); + } + throw std::runtime_error("Failed to intern annotation keys."); + }(); + return *keys; +} + +enum class DictLookupStatus { found, missing, error }; + +// Borrowed lookup with an explicit absent-versus-error outcome. +// +// Annotation parsing remains best-effort, matching the previous per-field +// exception handling. The status prevents a failed preferred crowd-key lookup +// from being treated as absent and falling back to the legacy spelling. +// +// PyDict_GetItemWithError is a C-level slot read: it assumes annotations are +// plain dicts (as produced from COCO/LVIS JSON) and does not honour a dict +// subclass that overrides __getitem__/__missing__, unlike the previous +// contains()+operator[] path. +struct DictLookup { + PyObject* value; + DictLookupStatus status; +}; + +inline DictLookup dict_get(const py::dict& mapping, PyObject* key) { + PyObject* value = PyDict_GetItemWithError(mapping.ptr(), key); + if (value != nullptr) { + return {value, DictLookupStatus::found}; + } + if (value == nullptr && PyErr_Occurred()) { + PyErr_Clear(); + return {nullptr, DictLookupStatus::error}; + } + return {nullptr, DictLookupStatus::missing}; +} + +} // namespace + // Helper method to convert py::object to InstanceAnnotation InstanceAnnotation LightweightDataset::parse_py_annotation( const py::object& ann) const { @@ -131,49 +245,61 @@ InstanceAnnotation LightweightDataset::parse_py_annotation( // Extract values from Python dict with safe type handling py::dict ann_dict = ann.cast(); - - try { - if (ann_dict.contains("id")) { - id = ann_dict["id"].cast(); + const AnnotationKeys& keys = annotation_keys(); + + // Each field keeps its own try/catch so that one unconvertible value + // leaves that field at its default without discarding the others. + if (const DictLookup lookup = dict_get(ann_dict, keys.id); + lookup.value != nullptr) { + try { + id = py::handle(lookup.value).cast(); + } catch (const std::exception&) { } - } catch (const std::exception&) { } - try { - if (ann_dict.contains("score")) { - score = ann_dict["score"].cast(); + if (const DictLookup lookup = dict_get(ann_dict, keys.score); + lookup.value != nullptr) { + try { + score = py::handle(lookup.value).cast(); + } catch (const std::exception&) { } - } catch (const std::exception&) { } - try { - if (ann_dict.contains("area")) { - area = ann_dict["area"].cast(); + if (const DictLookup lookup = dict_get(ann_dict, keys.area); + lookup.value != nullptr) { + try { + area = py::handle(lookup.value).cast(); + } catch (const std::exception&) { } - } catch (const std::exception&) { } - try { - if (ann_dict.contains("is_crowd")) { - is_crowd = ann_dict["is_crowd"].cast(); - } else if (ann_dict.contains("iscrowd")) { - is_crowd = ann_dict["iscrowd"].cast(); + // "is_crowd" wins when present; "iscrowd" is only consulted if the + // preferred spelling is absent, matching the original else-if. + DictLookup crowd_lookup = dict_get(ann_dict, keys.is_crowd); + if (crowd_lookup.status == DictLookupStatus::missing) { + crowd_lookup = dict_get(ann_dict, keys.iscrowd); + } + if (crowd_lookup.value != nullptr) { + try { + is_crowd = py::handle(crowd_lookup.value).cast(); + } catch (const std::exception&) { } - } catch (const std::exception&) { } - try { - if (ann_dict.contains("ignore")) { - ignore = ann_dict["ignore"].cast(); + if (const DictLookup lookup = dict_get(ann_dict, keys.ignore); + lookup.value != nullptr) { + try { + ignore = py::handle(lookup.value).cast(); + } catch (const std::exception&) { } - } catch (const std::exception&) { } - try { - if (ann_dict.contains("lvis_mark")) { - lvis_mark = ann_dict["lvis_mark"].cast(); + if (const DictLookup lookup = dict_get(ann_dict, keys.lvis_mark); + lookup.value != nullptr) { + try { + lvis_mark = py::handle(lookup.value).cast(); + } catch (const std::exception&) { } - } catch (const std::exception&) { } // Construct and return the annotation. diff --git a/tests/test_cpp_safety_regressions.py b/tests/test_cpp_safety_regressions.py index 73ecbd8..99871c2 100644 --- a/tests/test_cpp_safety_regressions.py +++ b/tests/test_cpp_safety_regressions.py @@ -106,3 +106,109 @@ def test_accumulate_rejects_nan_detection_score_before_sorting(): with pytest.raises(ValueError, match="Detection scores must not be NaN"): _eval.COCOevalAccumulate(params, [evaluation]) + + +def _evaluate_native_annotation(annotation: dict, detection_ids: tuple[int, ...] = (100,)) -> tuple: + """Serialize the native result for one parsed ground truth annotation.""" + ground_truth = _eval.Dataset() + detections = _eval.Dataset() + ground_truth.append_ref(1.0, 1.0, {"id": 1, "area": 100.0, **annotation}) + + for index, detection_id in enumerate(detection_ids): + detections.append_ref( + 1.0, + 1.0, + {"id": detection_id, "score": 1.0 - index / 10.0, "area": 100.0}, + ) + + return _eval.COCOevalEvaluateImages( + [[0.0, 100000.0]], + 10, + [0.5], + [[[[1.0] for _ in detection_ids]]], + ground_truth, + detections, + [1.0], + [1.0], + True, + )[0].__getstate__() + + +class TestAnnotationFieldParsing: + """Field lookup in the native annotation parser.""" + + @pytest.mark.parametrize( + ("annotation", "expected_matches"), + [ + pytest.param({"is_crowd": 1, "iscrowd": 0}, [1, 1], id="preferred-crowd-key-wins"), + pytest.param({"is_crowd": 0, "iscrowd": 1}, [1, 0], id="preferred-non-crowd-key-wins"), + pytest.param({"iscrowd": 1}, [1, 1], id="legacy-crowd-key-fallback"), + ], + ) + def test_preferred_crowd_key_controls_repeated_matches(self, annotation, expected_matches): + """The native parser honors ``is_crowd`` before ``iscrowd``. + + Two detections make the parsed crowd flag observable: a crowd ground + truth can match both detections, while a regular ground truth cannot. + """ + state = _evaluate_native_annotation(annotation, (100, 101)) + + assert state[0] == expected_matches + + def test_raw_unconvertible_ignore_falls_back_to_default(self): + """An unconvertible raw ``ignore`` value remains non-ignored. + + This bypasses Python evaluator preparation, which otherwise + overwrites the annotation before the native parser sees it. + """ + state = _evaluate_native_annotation({"ignore": object()}) + + assert state[3] == [False] + + def test_failed_crowd_lookup_keeps_default_without_falling_back(self): + """A failed preferred-key lookup is not treated as absent.""" + + class ErrorKey(str): + def __hash__(self): + return hash("is_crowd") + + def __eq__(self, other): + raise RuntimeError("crowd lookup failed") + + state = _evaluate_native_annotation({ErrorKey("is_crowd"): 1, "iscrowd": 1}, (100, 101)) + + assert state[0] == [1, 0] + + def test_absent_optional_fields_are_tolerated(self): + """Omitting every optional field still evaluates. + + The base annotation carries no "score", "ignore", or + "lvis_mark"; each must read as its default rather than as a + missing-key error. + """ + assert _evaluate_native_annotation({})[0] == [1] + + def test_interned_field_keys_survive_repeated_parsing(self): + """The process-global interned annotation keys are never freed by + parsing. + + ``AnnotationKeys`` owns seven interned ``PyObject*`` and defines a + destructor. A ``static_assert`` in ``dataset.cpp`` keeps the type + non-copyable and non-movable so the destructor can never run twice + and double ``Py_XDECREF`` those keys; this is the runtime backstop. + A regressed build that double-freed an interned key would, after + enough parsing, resurrect it as a freed/garbage object and mis-read + or crash on every field lookup. + + Parsing the same crowd annotation many times keeps the interned + keys under sustained lookup churn. The ``is_crowd``-before- + ``iscrowd`` precedence stays observable (a crowd ground truth + matches both detections) only while every interned key is still a + live string, so a stable result across the whole run means no key + was freed underneath the parser. + """ + crowd = {"is_crowd": 1, "iscrowd": 0, "ignore": 0, "lvis_mark": 0} + for _ in range(300): + state = _evaluate_native_annotation(crowd, (100, 101)) + assert state[0] == [1, 1] + assert state[3] == [False]