Skip to content

Commit

Permalink
Remove delete identifiers storage.
Browse files Browse the repository at this point in the history
With reference counted identifiers, there's no need to leak them into
some global.
  • Loading branch information
1uc committed Sep 25, 2023
1 parent fac2aaf commit 08df37c
Show file tree
Hide file tree
Showing 9 changed files with 6 additions and 64 deletions.
4 changes: 0 additions & 4 deletions src/neuron/container/memory_usage.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,11 @@ struct MemoryUsage {
ModelMemoryUsage model{};
cache::ModelMemoryUsage cache_model{};
VectorMemoryUsage stable_pointers{};
VectorMemoryUsage stable_identifiers{};

const MemoryUsage& operator+=(const MemoryUsage& other) {
model += other.model;
cache_model += other.cache_model;
stable_pointers += other.stable_pointers;
stable_identifiers += other.stable_identifiers;

return *this;
}
Expand All @@ -136,7 +134,6 @@ struct MemoryUsage {
auto total = model.compute_total();
total += cache_model.compute_total();
total += stable_pointers;
total += stable_identifiers;

return total;
}
Expand Down Expand Up @@ -174,7 +171,6 @@ struct MemoryUsageSummary {
add(memory_usage.model);
add(memory_usage.cache_model);
add(leaked, memory_usage.stable_pointers);
add(leaked, memory_usage.stable_identifiers);
}

private:
Expand Down
1 change: 1 addition & 0 deletions src/neuron/container/non_owning_soa_identifier.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace neuron::container {
struct field_index {
int field{}, array_index{};
};

inline constexpr std::size_t invalid_row = std::numeric_limits<std::size_t>::max();

/**
Expand Down
11 changes: 0 additions & 11 deletions src/neuron/container/soa_identifier.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,6 @@
#include <vector>

namespace neuron::container {
namespace detail {
/**
* @brief The vector in which dying owning_identifier std::size_t's live.
*
* This is defined in container.cpp to avoid multiple-definition issues.
*/
extern std::vector<std::unique_ptr<std::size_t>>* identifier_defer_delete_storage;
VectorMemoryUsage compute_identifier_defer_delete_storage_size();

} // namespace detail

/**
* @brief A non-owning permutation-stable identifier for a entry in a container.
* @tparam Storage The type of the referred-to container. This might be a type
Expand Down
7 changes: 0 additions & 7 deletions src/neuron/model_data.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,6 @@ struct Model {
void shrink_to_fit() {
m_node_data.shrink_to_fit();
apply_to_mechanisms([](auto& mech_data) { mech_data.shrink_to_fit(); });

m_identifier_ptrs_for_deferred_deletion.shrink_to_fit();
}

private:
Expand Down Expand Up @@ -157,11 +155,6 @@ struct Model {
* @brief Backing storage for defer_delete helper.
*/
std::vector<void*> m_ptrs_for_deferred_deletion{};

/**
* @brief Backing storage for global identifier_defer_delete_storage.
*/
std::vector<std::unique_ptr<std::size_t>> m_identifier_ptrs_for_deferred_deletion{};
};

struct model_sorted_token {
Expand Down
9 changes: 1 addition & 8 deletions src/nrniv/memory_usage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ cache::ModelMemoryUsage memory_usage(const neuron::cache::Model& model) {
MemoryUsage local_memory_usage() {
return MemoryUsage{memory_usage(model()),
memory_usage(neuron::cache::model),
detail::compute_defer_delete_storage_size(),
detail::compute_identifier_defer_delete_storage_size()};
detail::compute_defer_delete_storage_size()};
}

namespace detail {
Expand All @@ -62,10 +61,6 @@ VectorMemoryUsage compute_defer_delete_storage_size(std::vector<T> const* const
}


VectorMemoryUsage compute_identifier_defer_delete_storage_size() {
return compute_defer_delete_storage_size(identifier_defer_delete_storage, sizeof(std::size_t));
}

VectorMemoryUsage compute_defer_delete_storage_size() {
return compute_defer_delete_storage_size(defer_delete_storage, sizeof(void*));
}
Expand Down Expand Up @@ -105,7 +100,6 @@ std::string format_memory_usage(const MemoryUsage& usage) {
const auto& model = usage.model;
const auto& cache_model = usage.cache_model;
const auto& stable_pointers = usage.stable_pointers;
const auto& stable_identifiers = usage.stable_identifiers;
const auto& total = usage.compute_total();
const auto& summary = MemoryUsageSummary(usage);

Expand All @@ -124,7 +118,6 @@ std::string format_memory_usage(const MemoryUsage& usage) {
os << " threads " << format_memory_usage(cache_model.threads) << "\n";
os << " mechanisms " << format_memory_usage(cache_model.mechanisms) << "\n";
os << "deferred deletion \n";
os << " stable_identifiers " << format_memory_usage(stable_identifiers) << "\n";
os << " stable_pointers " << format_memory_usage(stable_pointers) << "\n";
os << "\n";
os << "total " << format_memory_usage(total) << "\n";
Expand Down
7 changes: 0 additions & 7 deletions src/nrnoc/container.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,8 @@ Model::Model() {
// needs some re-organisation if we ever want to support multiple Model instances
assert(!container::detail::defer_delete_storage);
container::detail::defer_delete_storage = &m_ptrs_for_deferred_deletion;
assert(!container::detail::identifier_defer_delete_storage);
container::detail::identifier_defer_delete_storage = &m_identifier_ptrs_for_deferred_deletion;
}
Model::~Model() {
assert(container::detail::identifier_defer_delete_storage ==
&m_identifier_ptrs_for_deferred_deletion);
container::detail::identifier_defer_delete_storage = nullptr;
assert(container::detail::defer_delete_storage == &m_ptrs_for_deferred_deletion);
container::detail::defer_delete_storage = nullptr;
std::for_each(m_ptrs_for_deferred_deletion.begin(),
Expand Down Expand Up @@ -96,8 +91,6 @@ std::ostream& operator<<(std::ostream& os, generic_data_handle const& dh) {
}
} // namespace neuron::container
namespace neuron::container::detail {
// See neuron/container/soa_identifier.hpp
std::vector<std::unique_ptr<std::size_t>>* identifier_defer_delete_storage{};
// See neuron/container/soa_container.hpp
std::vector<void*>* defer_delete_storage{};
} // namespace neuron::container::detail
Expand Down
1 change: 1 addition & 0 deletions src/nrnoc/section.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ struct Node {
// neuron::container::handle::Node, but as an intermediate measure we can
// add one of those as a member and forward some access/modifications to it.
neuron::container::Node::owning_handle _node_handle{neuron::model().node_data()};

[[nodiscard]] auto id() {
return _node_handle.id();
}
Expand Down
20 changes: 3 additions & 17 deletions test/unit_tests/container/container.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,20 +150,6 @@ TEST_CASE("soa::get_num_variables", "[Neuron][data_structures]") {
CHECK(data.get_num_variables<field::DOn>() == 1ul);
}

TEST_CASE("Identifier defer delete ", "[Neuron][internal][data_structures]") {
storage data;

REQUIRE(detail::identifier_defer_delete_storage != nullptr);
auto usage_before = detail::compute_identifier_defer_delete_storage_size();
{ owning_handle instance{data}; }
auto usage_after = detail::compute_identifier_defer_delete_storage_size();

CHECK(usage_after.size - usage_before.size > 0);
CHECK(usage_after.capacity > 0);
CHECK(usage_before.size <= usage_before.capacity);
CHECK(usage_after.size <= usage_after.capacity);
}

TEST_CASE("defer delete storage pointer", "[Neuron][internal][data_structures]") {
REQUIRE(detail::defer_delete_storage != nullptr);

Expand Down Expand Up @@ -279,7 +265,7 @@ neuron::container::MemoryUsage dummy_memory_usage() {
auto stable_pointers = neuron::container::VectorMemoryUsage(7, 17);
auto stable_identifiers = neuron::container::VectorMemoryUsage(8, 18);

auto memory_usage = MemoryUsage{model, cache_model, stable_pointers, stable_identifiers};
auto memory_usage = MemoryUsage{model, cache_model, stable_pointers};

return memory_usage;
}
Expand All @@ -288,8 +274,8 @@ neuron::container::MemoryUsage dummy_memory_usage() {
TEST_CASE("total memory usage", "[Neuron][internal][data_structures]") {
auto memory_usage = dummy_memory_usage();
auto total = memory_usage.compute_total();
CHECK(total.size == (8 * 9) / 2);
CHECK(total.capacity == total.size + 8 * 10);
CHECK(total.size == (7 * 8) / 2);
CHECK(total.capacity == total.size + 7 * 10);
}

TEST_CASE("memory usage summary", "[Neuron][data_structures]") {
Expand Down
10 changes: 0 additions & 10 deletions test/unit_tests/container/node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -273,16 +273,6 @@ TEST_CASE("SOA-backed Node structure", "[Neuron][data_structures][node]") {
}
}
}
GIVEN("A node that is deleted without an active deferred-deletion vector") {
auto* const old = std::exchange(neuron::container::detail::identifier_defer_delete_storage,
nullptr);
// Because identifier_defer_delete_storage is nullptr, deleting `node` will delete the
// heap-allocated std::size_t that the data handles depend on. This touches an otherwise
// uncovered code path in soa_identifier.hpp. Meaningfully checking the right code path was
// followed seems excessively complicated.
{ ::Node node{}; }
neuron::container::detail::identifier_defer_delete_storage = old;
}
GIVEN("A series of nodes with increasing integer voltages") {
using neuron::test::get_node_voltages;
auto nodes_and_voltages = neuron::test::get_nodes_and_reference_voltages();
Expand Down

0 comments on commit 08df37c

Please sign in to comment.