From 0773d3256762e2fab6214e5a99df996441bcd755 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Sun, 4 Feb 2024 18:27:46 +0100 Subject: [PATCH 01/19] Speedups: remove dependency on c++ --- .gitignore | 1 + _speedups.pyx | 37 +++++++++++++--- _speedups.pyxbld | 8 ++-- intset.h | 108 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 146 insertions(+), 8 deletions(-) create mode 100644 intset.h diff --git a/.gitignore b/.gitignore index 0bba6f17264b..5686f43de380 100644 --- a/.gitignore +++ b/.gitignore @@ -178,6 +178,7 @@ dmypy.json cython_debug/ # Cython intermediates +_speedups.c _speedups.cpp _speedups.html diff --git a/_speedups.pyx b/_speedups.pyx index b4167ec5aa1e..fd3bdf3c827e 100644 --- a/_speedups.pyx +++ b/_speedups.pyx @@ -1,5 +1,6 @@ #cython: language_level=3 -#distutils: language = c++ +#distutils: language = c +#distutils: depends = intset.h """ Provides faster implementation of some core parts. @@ -13,7 +14,6 @@ from cpython cimport PyObject from typing import Any, Dict, Iterable, Iterator, Generator, Sequence, Tuple, TypeVar, Union, Set, List, TYPE_CHECKING from cymem.cymem cimport Pool from libc.stdint cimport int64_t, uint32_t -from libcpp.set cimport set as std_set from collections import defaultdict cdef extern from *: @@ -31,6 +31,27 @@ ctypedef int64_t ap_id_t cdef ap_player_t MAX_PLAYER_ID = 1000000 # limit the size of indexing array cdef size_t INVALID_SIZE = (-1) # this is all 0xff... adding 1 results in 0, but it's not negative +# configure INTSET for player +cdef extern from *: + """ + #define INTSET_NAME ap_player_set + #define INTSET_TYPE uint32_t // has to match ap_player_t + """ + +# create INTSET for player +cdef extern from "intset.h": + """ + #undef INTSET_NAME + #undef INTSET_TYPE + """ + ctypedef struct ap_player_set: + pass + + ap_player_set* ap_player_set_new(size_t bucket_count) nogil + void ap_player_set_free(ap_player_set* set) nogil + bint ap_player_set_add(ap_player_set* set, ap_player_t val) nogil + bint ap_player_set_contains(ap_player_set* set, ap_player_t val) nogil + cdef struct LocationEntry: # layout is so that @@ -185,7 +206,7 @@ cdef class LocationStore: def find_item(self, slots: Set[int], seeked_item_id: int) -> Generator[Tuple[int, int, int, int, int], None, None]: cdef ap_id_t item = seeked_item_id cdef ap_player_t receiver - cdef std_set[ap_player_t] receivers + cdef ap_player_set* receivers cdef size_t slot_count = len(slots) if slot_count == 1: # specialized implementation for single slot @@ -197,13 +218,19 @@ cdef class LocationStore: yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags elif slot_count: # generic implementation with lookup in set + receivers = ap_player_set_new(min(1000, slot_count)) + if not receivers: + raise MemoryError() for receiver in slots: - receivers.insert(receiver) + if not ap_player_set_add(receivers, receiver): + ap_player_set_free(receivers) + raise MemoryError() with nogil: for entry in self.entries[:self.entry_count]: - if entry.item == item and receivers.count(entry.receiver): + if entry.item == item and ap_player_set_contains(receivers, entry.receiver): with gil: yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags + ap_player_set_free(receivers) def get_for_player(self, slot: int) -> Dict[int, Set[int]]: cdef ap_player_t receiver = slot diff --git a/_speedups.pyxbld b/_speedups.pyxbld index e1fe19b2efc6..974eaed03b6a 100644 --- a/_speedups.pyxbld +++ b/_speedups.pyxbld @@ -1,8 +1,10 @@ -# This file is required to get pyximport to work with C++. -# Switching from std::set to a pure C implementation is still on the table to simplify everything. +# This file is used when doing pyximport +import os def make_ext(modname, pyxfilename): from distutils.extension import Extension return Extension(name=modname, sources=[pyxfilename], - language='c++') + depends=["intset.h"], + include_dirs=[os.getcwd()], + language="c") diff --git a/intset.h b/intset.h new file mode 100644 index 000000000000..ef5177f3d47d --- /dev/null +++ b/intset.h @@ -0,0 +1,108 @@ +// A specialized unordered_set implementation for literals, where bucket_count +// is defined at initialization rather than increased automatically. +#include +#include +#include +#include + +#ifndef INTSET_NAME +#error "Please #define INTSET_NAME ... before including intset.h" +#endif + +#ifndef INTSET_TYPE +#error "Please #define INTSET_TYPE ... before including intset.h" +#endif + +// macros to generate unique names from INTSET_NAME +#ifndef INTSET_CONCAT +#define INTSET_CONCAT_(a, b) a ## b +#define INTSET_CONCAT(a, b) INTSET_CONCAT_(a, b) +#define INTSET_FUNC_(a, b) INTSET_CONCAT(a, _ ## b) +#endif + +#define INTSET_FUNC(name) INTSET_FUNC_(INTSET_NAME, name) +#define INTSET_BUCKET INTSET_CONCAT(INTSET_NAME, Bucket) + +typedef struct { + size_t count; + union { + INTSET_TYPE val; + INTSET_TYPE *data; + }; +} INTSET_BUCKET; + +typedef struct { + size_t bucket_count; + INTSET_BUCKET buckets[]; +} INTSET_NAME; + +static INTSET_NAME *INTSET_FUNC(new)(size_t buckets) +{ + if (buckets < 1) + buckets = 1; + size_t size = sizeof(INTSET_NAME) + buckets * sizeof(INTSET_BUCKET); + INTSET_NAME *set = (INTSET_NAME*)malloc(size); + memset(set, 0, size); + set->bucket_count = buckets; + return set; +} + +static void INTSET_FUNC(free)(INTSET_NAME *set) +{ + size_t i; + for (i = 0; i < set->bucket_count; i++) { + if (set->buckets[i].count > 1) + free(set->buckets[i].data); + } + free(set); +} + +static bool INTSET_FUNC(contains)(INTSET_NAME *set, INTSET_TYPE val) +{ + size_t i; + INTSET_BUCKET* bucket = set->buckets + ((size_t)val % set->bucket_count); + if (bucket->count == 1) + return bucket->val == val; + for (i = 0; i < bucket->count; ++i) { + if (bucket->data[i] == val) + return true; + } + return false; +} + +static bool INTSET_FUNC(add)(INTSET_NAME *set, INTSET_TYPE val) +{ + INTSET_BUCKET* bucket; + size_t new_bucket_size; + + if (INTSET_FUNC(contains)(set, val)) + return true; /* ok */ + + bucket = set->buckets + ((size_t)val % set->bucket_count); + if (bucket->count == 0 && sizeof(INTSET_TYPE) <= sizeof(INTSET_TYPE*)) { + bucket->val = val; + bucket->count = 1; + } else if (bucket->count == 1 && sizeof(INTSET_TYPE) <= sizeof(INTSET_TYPE*)) { + INTSET_TYPE old = bucket->val; + bucket->data = (INTSET_TYPE*)malloc(2 * sizeof(INTSET_TYPE)); + if (!bucket->data) { + bucket->count = 0; + return false; /* error */ + } + bucket->data[0] = old; + bucket->data[1] = val; + bucket->count = 2; + } else { + new_bucket_size = (bucket->count + 1) * sizeof(INTSET_TYPE); + bucket->data = (INTSET_TYPE*)realloc(bucket->data, new_bucket_size); + if (!bucket->data) { + bucket->count = 0; + return false; /* error */ + } + bucket->data[bucket->count++] = val; + } + return true; /* success */ +} + +#undef INTSET_FUNC +#undef INTSET_BUCKET From d326147ede7e7f384f7535e6507ec865d715f952 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Sun, 4 Feb 2024 18:39:33 +0100 Subject: [PATCH 02/19] Speedups: intset: handle malloc failing --- intset.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/intset.h b/intset.h index ef5177f3d47d..0f150eb01274 100644 --- a/intset.h +++ b/intset.h @@ -42,6 +42,8 @@ static INTSET_NAME *INTSET_FUNC(new)(size_t buckets) buckets = 1; size_t size = sizeof(INTSET_NAME) + buckets * sizeof(INTSET_BUCKET); INTSET_NAME *set = (INTSET_NAME*)malloc(size); + if (!set) + return NULL; memset(set, 0, size); set->bucket_count = buckets; return set; From e3887473843546fa666af8ef1a2a0ae04006d5d6 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Sun, 4 Feb 2024 19:22:26 +0100 Subject: [PATCH 03/19] Speedups: intset: fix corner case for int64 on 32bit systems original idea was to only use bucket->val if intbuckets + ((size_t)val % set->bucket_count); - if (bucket->count == 0 && sizeof(INTSET_TYPE) <= sizeof(INTSET_TYPE*)) { + if (bucket->count == 0) { bucket->val = val; bucket->count = 1; - } else if (bucket->count == 1 && sizeof(INTSET_TYPE) <= sizeof(INTSET_TYPE*)) { + } else if (bucket->count == 1) { INTSET_TYPE old = bucket->val; bucket->data = (INTSET_TYPE*)malloc(2 * sizeof(INTSET_TYPE)); if (!bucket->data) { From 88785d7f9166b651343830be0032cd12cf0221ad Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Tue, 6 Feb 2024 09:44:01 +0100 Subject: [PATCH 04/19] Speedups: add size comment to player_set bucket configuration --- _speedups.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/_speedups.pyx b/_speedups.pyx index fd3bdf3c827e..02bf838f241e 100644 --- a/_speedups.pyx +++ b/_speedups.pyx @@ -218,7 +218,7 @@ cdef class LocationStore: yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags elif slot_count: # generic implementation with lookup in set - receivers = ap_player_set_new(min(1000, slot_count)) + receivers = ap_player_set_new(min(1023, slot_count)) # limit top level struct to 16KB if not receivers: raise MemoryError() for receiver in slots: From 1694e3fcffad12d245b998fc4801331e38799ed3 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 7 Feb 2024 00:10:05 +0100 Subject: [PATCH 05/19] test: more tests for LocationStore.find_item --- test/netutils/test_location_store.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/netutils/test_location_store.py b/test/netutils/test_location_store.py index a7f117255faa..8d24c05c94de 100644 --- a/test/netutils/test_location_store.py +++ b/test/netutils/test_location_store.py @@ -86,12 +86,15 @@ def test_items(self) -> None: def test_find_item(self) -> None: self.assertEqual(sorted(self.store.find_item(set(), 99)), []) + self.assertEqual(sorted(self.store.find_item({7, 8, 9}, 99)), []) self.assertEqual(sorted(self.store.find_item({3}, 1)), []) self.assertEqual(sorted(self.store.find_item({5}, 99)), []) self.assertEqual(sorted(self.store.find_item({3}, 99)), [(4, 9, 99, 3, 0)]) self.assertEqual(sorted(self.store.find_item({3, 4}, 99)), [(3, 9, 99, 4, 0), (4, 9, 99, 3, 0)]) + self.assertEqual(sorted(self.store.find_item({2, 3, 4, 5}, 99)), + [(3, 9, 99, 4, 0), (4, 9, 99, 3, 0)]) def test_get_for_player(self) -> None: self.assertEqual(self.store.get_for_player(3), {4: {9}}) From b974b9a1b0c6d37d470948565db54b4f8c01ed9e Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 7 Feb 2024 00:10:24 +0100 Subject: [PATCH 06/19] test: require _speedups in CI This kind of tests that the build succeeds. --- test/netutils/test_location_store.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/netutils/test_location_store.py b/test/netutils/test_location_store.py index 8d24c05c94de..67534d3a2394 100644 --- a/test/netutils/test_location_store.py +++ b/test/netutils/test_location_store.py @@ -1,4 +1,5 @@ # Tests for _speedups.LocationStore and NetUtils._LocationStore +import os import typing import unittest import warnings @@ -7,6 +8,8 @@ State = typing.Dict[typing.Tuple[int, int], typing.Set[int]] RawLocations = typing.Dict[int, typing.Dict[int, typing.Tuple[int, int, int]]] +ci = bool(os.environ.get("CI")) # always set in GitHub actions + sample_data: RawLocations = { 1: { 11: (21, 2, 7), @@ -199,18 +202,20 @@ def setUp(self) -> None: super().setUp() -@unittest.skipIf(LocationStore is _LocationStore, "_speedups not available") +@unittest.skipIf(LocationStore is _LocationStore and not ci, "_speedups not available") class TestSpeedupsLocationStore(Base.TestLocationStore): """Run base method tests for cython implementation.""" def setUp(self) -> None: + self.assertFalse(LocationStore is _LocationStore, "Failed to load _speedups") self.store = LocationStore(sample_data) super().setUp() -@unittest.skipIf(LocationStore is _LocationStore, "_speedups not available") +@unittest.skipIf(LocationStore is _LocationStore and not ci, "_speedups not available") class TestSpeedupsLocationStoreConstructor(Base.TestLocationStoreConstructor): """Run base constructor tests and tests the additional constraints for cython implementation.""" def setUp(self) -> None: + self.assertFalse(LocationStore is _LocationStore, "Failed to load _speedups") self.type = LocationStore super().setUp() From 08d760a95d3fc7533adc32975037629e764cd08e Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 7 Feb 2024 00:53:52 +0100 Subject: [PATCH 07/19] test: even more tests for LocationStore.find_item --- test/netutils/test_location_store.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/test/netutils/test_location_store.py b/test/netutils/test_location_store.py index 67534d3a2394..f3e83989bea4 100644 --- a/test/netutils/test_location_store.py +++ b/test/netutils/test_location_store.py @@ -27,6 +27,9 @@ 3: { 9: (99, 4, 0), }, + 5: { + 9: (99, 5, 0), + } } empty_state: State = { @@ -48,14 +51,14 @@ class TestLocationStore(unittest.TestCase): store: typing.Union[LocationStore, _LocationStore] def test_len(self) -> None: - self.assertEqual(len(self.store), 4) + self.assertEqual(len(self.store), 5) self.assertEqual(len(self.store[1]), 3) def test_key_error(self) -> None: with self.assertRaises(KeyError): _ = self.store[0] with self.assertRaises(KeyError): - _ = self.store[5] + _ = self.store[6] locations = self.store[1] # no Exception with self.assertRaises(KeyError): _ = locations[7] @@ -74,7 +77,7 @@ def test_get(self) -> None: self.assertEqual(self.store[1].get(10, (None, None, None)), (None, None, None)) def test_iter(self) -> None: - self.assertEqual(sorted(self.store), [1, 2, 3, 4]) + self.assertEqual(sorted(self.store), [1, 2, 3, 4, 5]) self.assertEqual(len(self.store), len(sample_data)) self.assertEqual(list(self.store[1]), [11, 12, 13]) self.assertEqual(len(self.store[1]), len(sample_data[1])) @@ -88,16 +91,26 @@ def test_items(self) -> None: self.assertEqual(sorted(self.store[1].items())[0][1], self.store[1][11]) def test_find_item(self) -> None: + # empty player set self.assertEqual(sorted(self.store.find_item(set(), 99)), []) + # no such player, single + self.assertEqual(sorted(self.store.find_item({6}, 99)), []) + # no such player, set self.assertEqual(sorted(self.store.find_item({7, 8, 9}, 99)), []) + # no such item self.assertEqual(sorted(self.store.find_item({3}, 1)), []) - self.assertEqual(sorted(self.store.find_item({5}, 99)), []) + # valid matches self.assertEqual(sorted(self.store.find_item({3}, 99)), [(4, 9, 99, 3, 0)]) self.assertEqual(sorted(self.store.find_item({3, 4}, 99)), [(3, 9, 99, 4, 0), (4, 9, 99, 3, 0)]) - self.assertEqual(sorted(self.store.find_item({2, 3, 4, 5}, 99)), + self.assertEqual(sorted(self.store.find_item({2, 3, 4}, 99)), [(3, 9, 99, 4, 0), (4, 9, 99, 3, 0)]) + # test hash collision in set + self.assertEqual(sorted(self.store.find_item({3, 5}, 99)), + [(4, 9, 99, 3, 0), (5, 9, 99, 5, 0)]) + self.assertEqual(sorted(self.store.find_item(set(range(2048)), 13)), + [(1, 13, 13, 1, 0)]) def test_get_for_player(self) -> None: self.assertEqual(self.store.get_for_player(3), {4: {9}}) From 83fe8aa53faee5c155d151f49ee6d5eec52958d3 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Sat, 10 Feb 2024 17:30:24 +0100 Subject: [PATCH 08/19] Speedups: intset uniform comment style --- intset.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/intset.h b/intset.h index 7ff46a06333f..4eeaee4ba378 100644 --- a/intset.h +++ b/intset.h @@ -1,5 +1,6 @@ -// A specialized unordered_set implementation for literals, where bucket_count -// is defined at initialization rather than increased automatically. +/* A specialized unordered_set implementation for literals, where bucket_count + * is defined at initialization rather than increased automatically. + */ #include #include #include @@ -13,7 +14,7 @@ #error "Please #define INTSET_TYPE ... before including intset.h" #endif -// macros to generate unique names from INTSET_NAME +/* macros to generate unique names from INTSET_NAME */ #ifndef INTSET_CONCAT #define INTSET_CONCAT_(a, b) a ## b #define INTSET_CONCAT(a, b) INTSET_CONCAT_(a, b) From 9e0bdaa1989648c9eb896b540d2a7296b7def54c Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Sat, 10 Feb 2024 17:30:50 +0100 Subject: [PATCH 09/19] Speedups: intset: avoid memory leak when realloc fails --- intset.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/intset.h b/intset.h index 4eeaee4ba378..b185b31c397b 100644 --- a/intset.h +++ b/intset.h @@ -76,7 +76,6 @@ static bool INTSET_FUNC(contains)(INTSET_NAME *set, INTSET_TYPE val) static bool INTSET_FUNC(add)(INTSET_NAME *set, INTSET_TYPE val) { INTSET_BUCKET* bucket; - size_t new_bucket_size; if (INTSET_FUNC(contains)(set, val)) return true; /* ok */ @@ -89,19 +88,21 @@ static bool INTSET_FUNC(add)(INTSET_NAME *set, INTSET_TYPE val) INTSET_TYPE old = bucket->val; bucket->data = (INTSET_TYPE*)malloc(2 * sizeof(INTSET_TYPE)); if (!bucket->data) { - bucket->count = 0; + bucket->val = old; return false; /* error */ } bucket->data[0] = old; bucket->data[1] = val; bucket->count = 2; } else { + size_t new_bucket_size; + INTSET_TYPE* new_bucket_data; + new_bucket_size = (bucket->count + 1) * sizeof(INTSET_TYPE); - bucket->data = (INTSET_TYPE*)realloc(bucket->data, new_bucket_size); - if (!bucket->data) { - bucket->count = 0; + new_bucket_data = (INTSET_TYPE*)realloc(bucket->data, new_bucket_size); + if (!new_bucket_data) return false; /* error */ - } + bucket->data = new_bucket_data; bucket->data[bucket->count++] = val; } return true; /* success */ From f1bcbddc8bbb9221868406fda1430506ddb884c4 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Tue, 20 Feb 2024 09:22:28 +0100 Subject: [PATCH 10/19] Speedups: intset: make `gcc -pedantic -std=c99 -fanalyzer` without warnings Unnamed unions are not in C99, this got fixed. The overhead of setting count=0 is minimal or optimized-out and silences -fanalizer (see comment). --- intset.h | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/intset.h b/intset.h index b185b31c397b..1a29e689c3ba 100644 --- a/intset.h +++ b/intset.h @@ -23,13 +23,14 @@ #define INTSET_FUNC(name) INTSET_FUNC_(INTSET_NAME, name) #define INTSET_BUCKET INTSET_CONCAT(INTSET_NAME, Bucket) +#define INTSET_UNION INTSET_CONCAT(INTSET_NAME, Union) typedef struct { size_t count; - union { + union INTSET_UNION { INTSET_TYPE val; INTSET_TYPE *data; - }; + } v; } INTSET_BUCKET; typedef struct { @@ -39,13 +40,19 @@ typedef struct { static INTSET_NAME *INTSET_FUNC(new)(size_t buckets) { + size_t i, size; + INTSET_NAME *set; + if (buckets < 1) buckets = 1; - size_t size = sizeof(INTSET_NAME) + buckets * sizeof(INTSET_BUCKET); - INTSET_NAME *set = (INTSET_NAME*)malloc(size); + size = sizeof(INTSET_NAME) + buckets * sizeof(INTSET_BUCKET); + set = (INTSET_NAME*)malloc(size); if (!set) return NULL; - memset(set, 0, size); + memset(set, 0, size); /* gcc -fanalyzer does not understand this sets all buckets' count to 0 */ + for (i = 0; i < buckets; i++) { + set->buckets[i].count = 0; + } set->bucket_count = buckets; return set; } @@ -55,7 +62,7 @@ static void INTSET_FUNC(free)(INTSET_NAME *set) size_t i; for (i = 0; i < set->bucket_count; i++) { if (set->buckets[i].count > 1) - free(set->buckets[i].data); + free(set->buckets[i].v.data); } free(set); } @@ -63,11 +70,11 @@ static void INTSET_FUNC(free)(INTSET_NAME *set) static bool INTSET_FUNC(contains)(INTSET_NAME *set, INTSET_TYPE val) { size_t i; - INTSET_BUCKET* bucket = set->buckets + ((size_t)val % set->bucket_count); + INTSET_BUCKET* bucket = &set->buckets[(size_t)val % set->bucket_count]; if (bucket->count == 1) - return bucket->val == val; + return bucket->v.val == val; for (i = 0; i < bucket->count; ++i) { - if (bucket->data[i] == val) + if (bucket->v.data[i] == val) return true; } return false; @@ -80,33 +87,34 @@ static bool INTSET_FUNC(add)(INTSET_NAME *set, INTSET_TYPE val) if (INTSET_FUNC(contains)(set, val)) return true; /* ok */ - bucket = set->buckets + ((size_t)val % set->bucket_count); + bucket = &set->buckets[(size_t)val % set->bucket_count]; if (bucket->count == 0) { - bucket->val = val; + bucket->v.val = val; bucket->count = 1; } else if (bucket->count == 1) { - INTSET_TYPE old = bucket->val; - bucket->data = (INTSET_TYPE*)malloc(2 * sizeof(INTSET_TYPE)); - if (!bucket->data) { - bucket->val = old; + INTSET_TYPE old = bucket->v.val; + bucket->v.data = (INTSET_TYPE*)malloc(2 * sizeof(INTSET_TYPE)); + if (!bucket->v.data) { + bucket->v.val = old; return false; /* error */ } - bucket->data[0] = old; - bucket->data[1] = val; + bucket->v.data[0] = old; + bucket->v.data[1] = val; bucket->count = 2; } else { size_t new_bucket_size; INTSET_TYPE* new_bucket_data; new_bucket_size = (bucket->count + 1) * sizeof(INTSET_TYPE); - new_bucket_data = (INTSET_TYPE*)realloc(bucket->data, new_bucket_size); + new_bucket_data = (INTSET_TYPE*)realloc(bucket->v.data, new_bucket_size); if (!new_bucket_data) return false; /* error */ - bucket->data = new_bucket_data; - bucket->data[bucket->count++] = val; + bucket->v.data = new_bucket_data; + bucket->v.data[bucket->count++] = val; } return true; /* success */ } #undef INTSET_FUNC #undef INTSET_BUCKET +#undef INTSET_UNION From efe8858919146490daab63fce7f799af3707452d Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 21 Feb 2024 07:53:07 +0100 Subject: [PATCH 11/19] Speedups: don't leak memory in case of exception --- _speedups.pyx | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/_speedups.pyx b/_speedups.pyx index 02bf838f241e..4b083c2f9aef 100644 --- a/_speedups.pyx +++ b/_speedups.pyx @@ -221,16 +221,17 @@ cdef class LocationStore: receivers = ap_player_set_new(min(1023, slot_count)) # limit top level struct to 16KB if not receivers: raise MemoryError() - for receiver in slots: - if not ap_player_set_add(receivers, receiver): - ap_player_set_free(receivers) - raise MemoryError() - with nogil: - for entry in self.entries[:self.entry_count]: - if entry.item == item and ap_player_set_contains(receivers, entry.receiver): - with gil: - yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags - ap_player_set_free(receivers) + try: + for receiver in slots: + if not ap_player_set_add(receivers, receiver): + raise MemoryError() + with nogil: + for entry in self.entries[:self.entry_count]: + if entry.item == item and ap_player_set_contains(receivers, entry.receiver): + with gil: + yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags + finally: + ap_player_set_free(receivers) def get_for_player(self, slot: int) -> Dict[int, Set[int]]: cdef ap_player_t receiver = slot From 4f73de626e79bb4aae3832e0d010720363bf09b3 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 28 Feb 2024 00:05:55 +0100 Subject: [PATCH 12/19] Speedups: intset: validate alloc and free This won't happen in our cython, but it's still a good addition. --- intset.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/intset.h b/intset.h index 1a29e689c3ba..2baaaaf4ef23 100644 --- a/intset.h +++ b/intset.h @@ -45,6 +45,8 @@ static INTSET_NAME *INTSET_FUNC(new)(size_t buckets) if (buckets < 1) buckets = 1; + if ((SIZE_MAX - sizeof(INTSET_NAME)) / sizeof(INTSET_BUCKET) < buckets) + return NULL; size = sizeof(INTSET_NAME) + buckets * sizeof(INTSET_BUCKET); set = (INTSET_NAME*)malloc(size); if (!set) @@ -60,6 +62,8 @@ static INTSET_NAME *INTSET_FUNC(new)(size_t buckets) static void INTSET_FUNC(free)(INTSET_NAME *set) { size_t i; + if (!set) + return; for (i = 0; i < set->bucket_count; i++) { if (set->buckets[i].count > 1) free(set->buckets[i].v.data); From 1489cac7fc1d9ec8492cca04038692880ce28f78 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 28 Feb 2024 00:24:54 +0100 Subject: [PATCH 13/19] CI: add test framework for C/C++ code --- .github/workflows/ctest.yml | 54 ++++++++++++++++ test/cpp/CMakeLists.txt | 49 ++++++++++++++ test/cpp/README.md | 32 +++++++++ test/cpp/intset/CMakeLists.txt | 4 ++ test/cpp/intset/test_intset.cpp | 111 ++++++++++++++++++++++++++++++++ 5 files changed, 250 insertions(+) create mode 100644 .github/workflows/ctest.yml create mode 100644 test/cpp/CMakeLists.txt create mode 100644 test/cpp/README.md create mode 100644 test/cpp/intset/CMakeLists.txt create mode 100644 test/cpp/intset/test_intset.cpp diff --git a/.github/workflows/ctest.yml b/.github/workflows/ctest.yml new file mode 100644 index 000000000000..217b11387845 --- /dev/null +++ b/.github/workflows/ctest.yml @@ -0,0 +1,54 @@ +# Run CMake / CTest C++ unit tests + +name: ctest + +on: + push: + paths: + - '**.cc?' + - '**.cpp' + - '**.cxx' + - '**.hh?' + - '**.hpp' + - '**.hxx' + - '**.CMakeLists' + - '.github/workflows/ctest.yml' + pull_request: + paths: + - '**.cc?' + - '**.cpp' + - '**.cxx' + - '**.hh?' + - '**.hpp' + - '**.hxx' + - '**.CMakeLists' + - '.github/workflows/ctest.yml' + +jobs: + ctest: + runs-on: ${{ matrix.os }} + name: Test C++ ${{ matrix.os }} + + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest] + + steps: + - uses: actions/checkout@v4 + - uses: ilammy/msvc-dev-cmd@v1 + if: startsWith(matrix.os,'windows') + - uses: Bacondish2023/setup-googletest@v1 + with: + build-type: 'Release' + - name: Build tests + run: | + cd test/cpp + mkdir build + cmake -S . -B build/ -DCMAKE_BUILD_TYPE=Release + cmake --build build/ --config Release + ls + - name: Run tests + run: | + cd test + ctest --test-dir build/ -C Release --output-on-failure diff --git a/test/cpp/CMakeLists.txt b/test/cpp/CMakeLists.txt new file mode 100644 index 000000000000..927b7494dac4 --- /dev/null +++ b/test/cpp/CMakeLists.txt @@ -0,0 +1,49 @@ +cmake_minimum_required(VERSION 3.5) +project(ap-cpp-tests) + +enable_testing() + +find_package(GTest REQUIRED) + +if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") + add_definitions("/source-charset:utf-8") + set(CMAKE_CXX_FLAGS_DEBUG "/MTd") + set(CMAKE_CXX_FLAGS_RELEASE "/MT") +elseif (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + # enable static analysis for gcc + add_compile_options(-fanalyzer -Werror) + # disable stuff that gets triggered by googletest + add_compile_options(-Wno-analyzer-malloc-leak) + # enable asan for gcc + add_compile_options(-fsanitize=address) + add_link_options(-fsanitize=address) +endif () + +add_executable(test_default) + +target_include_directories(test_default + PRIVATE + ${GTEST_INCLUDE_DIRS} +) + +target_link_libraries(test_default + ${GTEST_BOTH_LIBRARIES} +) + +add_test( + NAME test_default + COMMAND test_default +) + +set_property( + TEST test_default + PROPERTY ENVIRONMENT "ASAN_OPTIONS=allocator_may_return_null=1" +) + +file(GLOB ITEMS *) +foreach(item ${ITEMS}) + if(IS_DIRECTORY ${item} AND EXISTS ${item}/CMakeLists.txt) + message(${item}) + add_subdirectory(${item}) + endif() +endforeach() diff --git a/test/cpp/README.md b/test/cpp/README.md new file mode 100644 index 000000000000..0e9cb7dbef1d --- /dev/null +++ b/test/cpp/README.md @@ -0,0 +1,32 @@ +# C++ tests + +Test framework for C and C++ code in AP. + +## Adding a Test + +### GoogleTest + +Adding GoogleTests is as simple as creating a directory with +* one or more `test_*.cpp` files that defines tests using + [GoogleTest API](https://google.github.io/googletest/) +* a `CMakeLists.txt` that adds the .cpp files to `test_default` target using + [target_sources](https://cmake.org/cmake/help/latest/command/target_sources.html) + +### CTest + +If either GoogleTest is not suitable for the test or the build flags / sources / libraries are incompatible, +you can add another CTest to project using add_target and add_test, similar to how it's done for `test_default`. + +## Running Tests + +* Install [CMake](https://cmake.org/). +* Build and/or install GoogleTest and make sure + [CMake can find it](https://cmake.org/cmake/help/latest/module/FindGTest.html), or + [create a parent `CMakeLists.txt` that fetches GoogleTest](https://google.github.io/googletest/quickstart-cmake.html). +* Enter the directory with the top-most `CMakeLists.txt` and run + ```sh + mkdir build + cmake -S . -B build/ -DCMAKE_BUILD_TYPE=Release + cmake --build build/ --config Release && \ + ctest --test-dir build/ -C Release --output-on-failure + ``` diff --git a/test/cpp/intset/CMakeLists.txt b/test/cpp/intset/CMakeLists.txt new file mode 100644 index 000000000000..175e0bd0b9e8 --- /dev/null +++ b/test/cpp/intset/CMakeLists.txt @@ -0,0 +1,4 @@ +target_sources(test_default + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/test_intset.cpp +) diff --git a/test/cpp/intset/test_intset.cpp b/test/cpp/intset/test_intset.cpp new file mode 100644 index 000000000000..870365f770c2 --- /dev/null +++ b/test/cpp/intset/test_intset.cpp @@ -0,0 +1,111 @@ +#include +#include +#include + +// uint32Set +#define INTSET_NAME uint32Set +#define INTSET_TYPE uint32_t +#include "../../../intset.h" +#undef INTSET_NAME +#undef INTSET_TYPE + +// int64Set +#define INTSET_NAME int64Set +#define INTSET_TYPE int64_t +#include "../../../intset.h" + + +TEST(IntsetTest, ZeroBuckets) +{ + // trying to allocate with zero buckets has to either fail or be functioning + uint32Set *set = uint32Set_new(0); + if (!set) + return; // failed -> OK + + EXPECT_FALSE(uint32Set_contains(set, 1)); + EXPECT_TRUE(uint32Set_add(set, 1)); + EXPECT_TRUE(uint32Set_contains(set, 1)); + uint32Set_free(set); +} + +TEST(IntsetTest, Duplicate) +{ + // adding the same number again can't fail + uint32Set *set = uint32Set_new(2); + ASSERT_TRUE(set); + EXPECT_TRUE(uint32Set_add(set, 0)); + EXPECT_TRUE(uint32Set_add(set, 0)); + EXPECT_TRUE(uint32Set_contains(set, 0)); + uint32Set_free(set); +} + +__attribute__((no_sanitize("address"))) +static int64Set* int64Set_new_unchecked(size_t buckets) +{ + return int64Set_new(buckets); +} + +TEST(IntsetTest, SetAllocFailure) +{ + // try to allocate 100TB of RAM, should fail and return NULL + if (sizeof(size_t) < 8) + GTEST_SKIP() << "Alloc error not testable on 32bit"; + int64Set *set = int64Set_new_unchecked(6250000000000ULL); + EXPECT_FALSE(set); + int64Set_free(set); +} + +TEST(IntsetTest, SetAllocOverflow) +{ + // try to overflow argument passed to malloc + int64Set *set = int64Set_new(std::numeric_limits::max()); + EXPECT_FALSE(set); + int64Set_free(set); +} + +TEST(IntsetTest, NullFree) +{ + // free(NULL) should not try to free buckets + uint32Set_free(NULL); + int64Set_free(NULL); +} + +TEST(IntsetTest, BucketRealloc) +{ + // add a couple of values to the same bucket to test growing the bucket + uint32Set* set = uint32Set_new(1); + ASSERT_TRUE(set); + EXPECT_FALSE(uint32Set_contains(set, 0)); + EXPECT_TRUE(uint32Set_add(set, 0)); + EXPECT_TRUE(uint32Set_contains(set, 0)); + for (uint32_t i = 1; i < 32; ++i) { + EXPECT_TRUE(uint32Set_add(set, i)); + EXPECT_TRUE(uint32Set_contains(set, i - 1)); + EXPECT_TRUE(uint32Set_contains(set, i)); + EXPECT_FALSE(uint32Set_contains(set, i + 1)); + } + uint32Set_free(set); +} + +TEST(IntSet, Max) +{ + constexpr auto n = std::numeric_limits::max(); + uint32Set *set = uint32Set_new(1); + ASSERT_TRUE(set); + EXPECT_FALSE(uint32Set_contains(set, n)); + EXPECT_TRUE(uint32Set_add(set, n)); + EXPECT_TRUE(uint32Set_contains(set, n)); + uint32Set_free(set); +} + +TEST(InsetTest, Negative) +{ + constexpr auto n = std::numeric_limits::min(); + static_assert(n < 0, "n not negative"); + int64Set *set = int64Set_new(1); + ASSERT_TRUE(set); + EXPECT_FALSE(int64Set_contains(set, n)); + EXPECT_TRUE(int64Set_add(set, n)); + EXPECT_TRUE(int64Set_contains(set, n)); + int64Set_free(set); +} From a1017ae3e2f9746378c62cbe3ac8889280df7ace Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 28 Feb 2024 00:27:19 +0100 Subject: [PATCH 14/19] CI: ctest: fix cwd --- .github/workflows/ctest.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ctest.yml b/.github/workflows/ctest.yml index 217b11387845..9492c83c9e53 100644 --- a/.github/workflows/ctest.yml +++ b/.github/workflows/ctest.yml @@ -50,5 +50,5 @@ jobs: ls - name: Run tests run: | - cd test + cd test/cpp ctest --test-dir build/ -C Release --output-on-failure From 47a75af974e18d895e604ae32d0699f3c30b9c6a Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 28 Feb 2024 00:41:34 +0100 Subject: [PATCH 15/19] Speedups: intset: ignore msvc warning --- intset.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/intset.h b/intset.h index 2baaaaf4ef23..fac84fb6f890 100644 --- a/intset.h +++ b/intset.h @@ -25,6 +25,12 @@ #define INTSET_BUCKET INTSET_CONCAT(INTSET_NAME, Bucket) #define INTSET_UNION INTSET_CONCAT(INTSET_NAME, Union) +#if defined(_MSC_VER) +#pragma warning(push) +#pragma warning(disable : 4200) +#endif + + typedef struct { size_t count; union INTSET_UNION { @@ -119,6 +125,11 @@ static bool INTSET_FUNC(add)(INTSET_NAME *set, INTSET_TYPE val) return true; /* success */ } + +#if defined(_MSC_VER) +#pragma warning(pop) +#endif + #undef INTSET_FUNC #undef INTSET_BUCKET #undef INTSET_UNION From 621c53e10447abcf4edb031581e0b106a7b8d13e Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 28 Feb 2024 00:47:21 +0100 Subject: [PATCH 16/19] Tests: intset: revert attempt at no-asan We solve this with env vars in ctest now, and this fails for msvc. --- test/cpp/intset/test_intset.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/test/cpp/intset/test_intset.cpp b/test/cpp/intset/test_intset.cpp index 870365f770c2..38403b604105 100644 --- a/test/cpp/intset/test_intset.cpp +++ b/test/cpp/intset/test_intset.cpp @@ -39,18 +39,12 @@ TEST(IntsetTest, Duplicate) uint32Set_free(set); } -__attribute__((no_sanitize("address"))) -static int64Set* int64Set_new_unchecked(size_t buckets) -{ - return int64Set_new(buckets); -} - TEST(IntsetTest, SetAllocFailure) { // try to allocate 100TB of RAM, should fail and return NULL if (sizeof(size_t) < 8) GTEST_SKIP() << "Alloc error not testable on 32bit"; - int64Set *set = int64Set_new_unchecked(6250000000000ULL); + int64Set *set = int64Set_new(6250000000000ULL); EXPECT_FALSE(set); int64Set_free(set); } From f17cc7180c20d8a078c37fdb3e87943872c7b054 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 28 Feb 2024 00:58:06 +0100 Subject: [PATCH 17/19] Test: cpp: docs: fix typo --- test/cpp/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cpp/README.md b/test/cpp/README.md index 0e9cb7dbef1d..685c17f767c3 100644 --- a/test/cpp/README.md +++ b/test/cpp/README.md @@ -15,7 +15,7 @@ Adding GoogleTests is as simple as creating a directory with ### CTest If either GoogleTest is not suitable for the test or the build flags / sources / libraries are incompatible, -you can add another CTest to project using add_target and add_test, similar to how it's done for `test_default`. +you can add another CTest to the project using add_target and add_test, similar to how it's done for `test_default`. ## Running Tests From c4310aac5b5f9413fcd1830c7f3126937dc5411f Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 28 Feb 2024 01:40:31 +0100 Subject: [PATCH 18/19] Test: cpp: docs: fix another typo --- test/cpp/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cpp/README.md b/test/cpp/README.md index 685c17f767c3..792b9be77e72 100644 --- a/test/cpp/README.md +++ b/test/cpp/README.md @@ -7,7 +7,7 @@ Test framework for C and C++ code in AP. ### GoogleTest Adding GoogleTests is as simple as creating a directory with -* one or more `test_*.cpp` files that defines tests using +* one or more `test_*.cpp` files that define tests using [GoogleTest API](https://google.github.io/googletest/) * a `CMakeLists.txt` that adds the .cpp files to `test_default` target using [target_sources](https://cmake.org/cmake/help/latest/command/target_sources.html) From 19109ccf41f708970cc0cd1fd630a1456d61d198 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 28 Feb 2024 02:05:24 +0100 Subject: [PATCH 19/19] Test: intset: proper bucket count for Negative test INTxx_MIN % 1 would not produce a negative number, so the test was flawed. --- test/cpp/intset/test_intset.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cpp/intset/test_intset.cpp b/test/cpp/intset/test_intset.cpp index 38403b604105..2f85bea960c4 100644 --- a/test/cpp/intset/test_intset.cpp +++ b/test/cpp/intset/test_intset.cpp @@ -96,7 +96,7 @@ TEST(InsetTest, Negative) { constexpr auto n = std::numeric_limits::min(); static_assert(n < 0, "n not negative"); - int64Set *set = int64Set_new(1); + int64Set *set = int64Set_new(3); ASSERT_TRUE(set); EXPECT_FALSE(int64Set_contains(set, n)); EXPECT_TRUE(int64Set_add(set, n));