From ded0ef6f6c775929ed94ef0415662258213b3bd9 Mon Sep 17 00:00:00 2001 From: Yorick van Pelt Date: Fri, 28 Jul 2023 10:49:21 +0200 Subject: [PATCH] nix_api_expr: switch to refcounting Remove GCRef, keep references in a map. Change to nix_gc_incref and nix_gc_decref, where users will mostly use nix_gc_decref. --- src/libexpr/nix_api_expr.cc | 48 +++++++++++++++++++---------- src/libexpr/nix_api_expr.h | 30 +++++++----------- src/libexpr/nix_api_expr_internal.h | 4 --- src/libexpr/nix_api_external.cc | 6 ++-- src/libexpr/nix_api_external.h | 7 ++--- src/libexpr/nix_api_value.cc | 28 ++++++++--------- src/libexpr/nix_api_value.h | 29 +++++++---------- 7 files changed, 71 insertions(+), 81 deletions(-) diff --git a/src/libexpr/nix_api_expr.cc b/src/libexpr/nix_api_expr.cc index 46c8835f2..1eb3693a2 100644 --- a/src/libexpr/nix_api_expr.cc +++ b/src/libexpr/nix_api_expr.cc @@ -16,6 +16,7 @@ #include "nix_api_util_internal.h" #ifdef HAVE_BOEHMGC +#include #define GC_INCLUDE_NEW 1 #include "gc_cpp.h" #endif @@ -100,27 +101,42 @@ State *nix_state_create(nix_c_context *context, const char **searchPath_c, void nix_state_free(State *state) { delete state; } -GCRef *nix_gc_ref(nix_c_context *context, void *obj) { - if (context) - context->last_err_code = NIX_OK; - try { -#if HAVE_BOEHMGC - return new (NoGC) GCRef{obj}; -#else - return new GCRef{obj}; -#endif +#ifdef HAVE_BOEHMGC +std::unordered_map< + const void *, unsigned int, std::hash, + std::equal_to, + traceable_allocator>> + nix_refcounts; + +std::mutex nix_refcount_lock; + +void nix_gc_incref(const void *p) { + std::scoped_lock lock(nix_refcount_lock); + auto f = nix_refcounts.find(p); + if (f != nix_refcounts.end()) { + f->second++; + } else { + nix_refcounts[p] = 1; } - NIXC_CATCH_ERRS_NULL } -void nix_gc_free(GCRef *ref) { -#if HAVE_BOEHMGC - GC_FREE(ref); -#else - delete ref; -#endif +void nix_gc_decref(const void *p) { + std::scoped_lock lock(nix_refcount_lock); + auto f = nix_refcounts.find(p); + if (f != nix_refcounts.end()) { + if (f->second == 1) + nix_refcounts.erase(f); + else + f->second--; + } + // todo: else { throw? } } +#else +void nix_gc_incref(const void *){}; +void nix_gc_decref(const void *){}; +#endif + void nix_gc_register_finalizer(void *obj, void *cd, void (*finalizer)(void *obj, void *cd)) { #ifdef HAVE_BOEHMGC diff --git a/src/libexpr/nix_api_expr.h b/src/libexpr/nix_api_expr.h index e53aa5cd9..ae2806343 100644 --- a/src/libexpr/nix_api_expr.h +++ b/src/libexpr/nix_api_expr.h @@ -26,14 +26,6 @@ typedef struct State State; // nix::EvalState * Owned by the GC. */ typedef void Value; // nix::Value -/** - * @brief Reference for the GC - * - * Nix uses a garbage collector that may not be able to see into - * your stack and heap. Keep GCRef objects around for every - * garbage-collected object that you want to keep alive. - */ -typedef struct GCRef GCRef; // void* // Function prototypes /** @@ -119,22 +111,22 @@ State *nix_state_create(nix_c_context *context, const char **searchPath, void nix_state_free(State *state); /** - * @brief Creates a new garbage collector reference. + * @brief Increase the GC refcount. * - * @param[out] context Optional, stores error information - * @param[in] obj The object to create a reference for. - * @return A new garbage collector reference or NULL on failure. + * The nix C api keeps alive objects by refcounting. + * When you're done with a refcounted pointer, call nix_gc_decref. + * + * Does not fail + * + * @param[in] object The object to keep alive */ -GCRef *nix_gc_ref(nix_c_context *context, void *obj); - +void nix_gc_incref(const void *); /** - * @brief Frees a garbage collector reference. + * @brief Decrease the GC refcount * - * Does not fail. - * - * @param[in] ref The reference to free. + * @param[in] object The object to stop referencing */ -void nix_gc_free(GCRef *ref); +void nix_gc_decref(const void *); /** * @brief Register a callback that gets called when the object is garbage diff --git a/src/libexpr/nix_api_expr_internal.h b/src/libexpr/nix_api_expr_internal.h index e9031d311..3ee3b18af 100644 --- a/src/libexpr/nix_api_expr_internal.h +++ b/src/libexpr/nix_api_expr_internal.h @@ -11,10 +11,6 @@ struct State { nix::EvalState state; }; -struct GCRef { - void *ptr; -}; - struct BindingsBuilder { nix::BindingsBuilder builder; }; diff --git a/src/libexpr/nix_api_external.cc b/src/libexpr/nix_api_external.cc index 1bf49f65a..d72adee80 100644 --- a/src/libexpr/nix_api_external.cc +++ b/src/libexpr/nix_api_external.cc @@ -169,8 +169,7 @@ public: }; ExternalValue *nix_create_external_value(nix_c_context *context, - NixCExternalValueDesc *desc, void *v, - GCRef *gc) { + NixCExternalValueDesc *desc, void *v) { if (context) context->last_err_code = NIX_OK; try { @@ -179,8 +178,7 @@ ExternalValue *nix_create_external_value(nix_c_context *context, (GC) #endif NixCExternalValue(*desc, v); - if (gc) - gc->ptr = ret; + nix_gc_incref(ret); return (ExternalValue *)ret; } NIXC_CATCH_ERRS_NULL diff --git a/src/libexpr/nix_api_external.h b/src/libexpr/nix_api_external.h index 4521f4736..45e95346b 100644 --- a/src/libexpr/nix_api_external.h +++ b/src/libexpr/nix_api_external.h @@ -159,18 +159,17 @@ typedef struct NixCExternalValueDesc { /** * @brief Create an external value, that can be given to nix_set_external * - * Pass a gcref to keep a reference. + * Owned by the GC. Use nix_gc_decref when you're done with the pointer. + * * @param[out] context Optional, stores error information * @param[in] desc a NixCExternalValueDesc, you should keep this alive as long * as the ExternalValue lives * @param[in] v the value to store - * @param[out] ref Optional, will store a reference to the returned value. * @returns external value, owned by the garbage collector * @see nix_set_external */ ExternalValue *nix_create_external_value(nix_c_context *context, - NixCExternalValueDesc *desc, void *v, - GCRef *ref); + NixCExternalValueDesc *desc, void *v); /** * @brief Extract the pointer from a nix c external value. diff --git a/src/libexpr/nix_api_value.cc b/src/libexpr/nix_api_value.cc index 74e8395fc..f34907ef1 100644 --- a/src/libexpr/nix_api_value.cc +++ b/src/libexpr/nix_api_value.cc @@ -32,8 +32,7 @@ static nix::Value &check_value_not_null(Value *value) { } PrimOp *nix_alloc_primop(nix_c_context *context, PrimOpFun fun, int arity, - const char *name, const char **args, const char *doc, - GCRef *ref) { + const char *name, const char **args, const char *doc) { if (context) context->last_err_code = NIX_OK; try { @@ -50,20 +49,18 @@ PrimOp *nix_alloc_primop(nix_c_context *context, PrimOpFun fun, int arity, if (args) for (size_t i = 0; args[i]; i++) p->args.emplace_back(*args); - if (ref) - ref->ptr = p; + nix_gc_incref(p); return (PrimOp *)p; } NIXC_CATCH_ERRS_NULL } -Value *nix_alloc_value(nix_c_context *context, State *state, GCRef *ref) { +Value *nix_alloc_value(nix_c_context *context, State *state) { if (context) context->last_err_code = NIX_OK; try { Value *res = state->state.allocValue(); - if (ref) - ref->ptr = res; + nix_gc_incref(res); return res; } NIXC_CATCH_ERRS_NULL @@ -204,19 +201,21 @@ ExternalValue *nix_get_external(nix_c_context *context, Value *value) { } Value *nix_get_list_byidx(nix_c_context *context, const Value *value, - unsigned int ix, GCRef *ref) { + unsigned int ix) { if (context) context->last_err_code = NIX_OK; try { auto &v = check_value_not_null(value); assert(v.type() == nix::nList); - return (Value *)v.listElems()[ix]; + auto *p = v.listElems()[ix]; + nix_gc_incref(p); + return (Value *)p; } NIXC_CATCH_ERRS_NULL } Value *nix_get_attr_byname(nix_c_context *context, const Value *value, - State *state, const char *name, GCRef *ref) { + State *state, const char *name) { if (context) context->last_err_code = NIX_OK; try { @@ -225,8 +224,7 @@ Value *nix_get_attr_byname(nix_c_context *context, const Value *value, nix::Symbol s = state->state.symbols.create(name); auto attr = v.attrs->get(s); if (attr) { - if (ref) - ref->ptr = attr->value; + nix_gc_incref(attr->value); return attr->value; } nix_set_err_msg(context, NIX_ERR_KEY, "missing attribute"); @@ -252,16 +250,14 @@ bool nix_has_attr_byname(nix_c_context *context, const Value *value, } Value *nix_get_attr_byidx(nix_c_context *context, const Value *value, - State *state, unsigned int i, const char **name, - GCRef *ref) { + State *state, unsigned int i, const char **name) { if (context) context->last_err_code = NIX_OK; try { auto &v = check_value_not_null(value); const nix::Attr &a = (*v.attrs)[i]; *name = ((const std::string &)(state->state.symbols[a.name])).c_str(); - if (ref) - ref->ptr = a.value; + nix_gc_incref(a.value); return a.value; } NIXC_CATCH_ERRS_NULL diff --git a/src/libexpr/nix_api_value.h b/src/libexpr/nix_api_value.h index 6aae5cf3c..8fa85b25d 100644 --- a/src/libexpr/nix_api_value.h +++ b/src/libexpr/nix_api_value.h @@ -33,7 +33,6 @@ typedef enum { // forward declarations typedef void Value; typedef struct State State; -typedef struct GCRef GCRef; // type defs /** @brief Stores an under-construction set of bindings * @@ -67,8 +66,7 @@ typedef void (*PrimOpFun)(State *state, int pos, Value **args, Value *v); /** @brief Allocate a primop * - * Owned by the GC - * Pass a gcref to keep a reference. + * Owned by the GC. Use nix_gc_decref when you're done with the pointer * * @param[out] context Optional, stores error information * @param[in] fun callback @@ -76,27 +74,23 @@ typedef void (*PrimOpFun)(State *state, int pos, Value **args, Value *v); * @param[in] name function name * @param[in] args array of argument names * @param[in] doc optional, documentation for this primop - * @param[out] ref Optional, will store a reference to the returned value. * @return primop, or null in case of errors * @see nix_set_primop */ PrimOp *nix_alloc_primop(nix_c_context *context, PrimOpFun fun, int arity, - const char *name, const char **args, const char *doc, - GCRef *ref); + const char *name, const char **args, const char *doc); // Function prototypes /** @brief Allocate a Nix value * - * Owned by the GC - * Pass a gcref to keep a reference. + * Owned by the GC. Use nix_gc_decref when you're done with the pointer * @param[out] context Optional, stores error information * @param[in] state nix evaluator state - * @param[out] ref Optional, will store a reference to the returned value. * @return value, or null in case of errors * */ -Value *nix_alloc_value(nix_c_context *context, State *state, GCRef *ref); +Value *nix_alloc_value(nix_c_context *context, State *state); /** @name Getters */ /**@{*/ @@ -167,27 +161,25 @@ ExternalValue *nix_get_external(nix_c_context *context, Value *); /** @brief Get the ix'th element of a list * - * Pass a gcref to keep a reference. + * Owned by the GC. Use nix_gc_decref when you're done with the pointer * @param[out] context Optional, stores error information * @param[in] value Nix value to inspect * @param[in] ix list element to get - * @param[out] ref Optional, will store a reference to the returned value. * @return value, NULL in case of errors */ Value *nix_get_list_byidx(nix_c_context *context, const Value *value, - unsigned int ix, GCRef *ref); + unsigned int ix); /** @brief Get an attr by name * - * Pass a gcref to keep a reference. + * Owned by the GC. Use nix_gc_decref when you're done with the pointer * @param[out] context Optional, stores error information * @param[in] value Nix value to inspect * @param[in] state nix evaluator state * @param[in] name attribute name - * @param[out] ref Optional, will store a reference to the returned value. * @return value, NULL in case of errors */ Value *nix_get_attr_byname(nix_c_context *context, const Value *value, - State *state, const char *name, GCRef *ref); + State *state, const char *name); /** @brief Check if an attribute name exists on a value * @param[out] context Optional, stores error information @@ -200,6 +192,8 @@ bool nix_has_attr_byname(nix_c_context *context, const Value *value, State *state, const char *name); /** @brief Get an attribute by index in the sorted bindings + * + * Owned by the GC. Use nix_gc_decref when you're done with the pointer * @param[out] context Optional, stores error information * @param[in] value Nix value to inspect * @param[in] state nix evaluator state @@ -208,8 +202,7 @@ bool nix_has_attr_byname(nix_c_context *context, const Value *value, * @return value, NULL in case of errors */ Value *nix_get_attr_byidx(nix_c_context *context, const Value *value, - State *state, unsigned int i, const char **name, - GCRef *ref); + State *state, unsigned int i, const char **name); /**@}*/ /** @name Setters */