perf(coco): intern annotation field keys

csrc/faster_eval_api/coco_eval/dataset.cpp

@@ -3,6 +3,7 @@
 #include <algorithm>
 #include <cstdint>
 #include <numeric>
+#include <type_traits>
 
 // clang-format off
 #include "cocoeval.h"
@@ -119,6 +120,119 @@ std::vector<py::dict> 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<AnnotationKeys>::value &&
+                  !std::is_move_constructible<AnnotationKeys>::value &&
+                  !std::is_copy_assignable<AnnotationKeys>::value &&
+                  !std::is_move_assignable<AnnotationKeys>::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<py::dict>();
-
-        try {
-                if (ann_dict.contains("id")) {
-                        id = ann_dict["id"].cast<uint64_t>();
+        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<uint64_t>();
+                } catch (const std::exception&) {
                 }
-        } catch (const std::exception&) {
         }
 
-        try {
-                if (ann_dict.contains("score")) {
-                        score = ann_dict["score"].cast<double>();
+        if (const DictLookup lookup = dict_get(ann_dict, keys.score);
+            lookup.value != nullptr) {
+                try {
+                        score = py::handle(lookup.value).cast<double>();
+                } catch (const std::exception&) {
                 }
-        } catch (const std::exception&) {
         }
 
-        try {
-                if (ann_dict.contains("area")) {
-                        area = ann_dict["area"].cast<double>();
+        if (const DictLookup lookup = dict_get(ann_dict, keys.area);
+            lookup.value != nullptr) {
+                try {
+                        area = py::handle(lookup.value).cast<double>();
+                } catch (const std::exception&) {
                 }
-        } catch (const std::exception&) {
         }
 
-        try {
-                if (ann_dict.contains("is_crowd")) {
-                        is_crowd = ann_dict["is_crowd"].cast<bool>();
-                } else if (ann_dict.contains("iscrowd")) {
-                        is_crowd = ann_dict["iscrowd"].cast<bool>();
+        // "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<bool>();
+                } catch (const std::exception&) {
                 }
-        } catch (const std::exception&) {
         }
 
-        try {
-                if (ann_dict.contains("ignore")) {
-                        ignore = ann_dict["ignore"].cast<bool>();
+        if (const DictLookup lookup = dict_get(ann_dict, keys.ignore);
+            lookup.value != nullptr) {
+                try {
+                        ignore = py::handle(lookup.value).cast<bool>();
+                } catch (const std::exception&) {
                 }
-        } catch (const std::exception&) {
         }
 
-        try {
-                if (ann_dict.contains("lvis_mark")) {
-                        lvis_mark = ann_dict["lvis_mark"].cast<bool>();
+        if (const DictLookup lookup = dict_get(ann_dict, keys.lvis_mark);
+            lookup.value != nullptr) {
+                try {
+                        lvis_mark = py::handle(lookup.value).cast<bool>();
+                } catch (const std::exception&) {
                 }
-        } catch (const std::exception&) {
         }
 
         // Construct and return the annotation.

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]