Skip to content

Commit 7af87d4

Browse files
petermmpetermm
committed
Refine persistent_term locking and get_all ownership
Use the project RWLock API directly for persistent_term instead of wrapping spinlock calls in local read/write lock macros. Also reduce the persistent_term critical sections by precomputing the bucket hash and allocating replacement entries before taking the write lock. Finally, stop copying keys in persistent_term:get/0. The returned list now allocates only the outer list and tuple cells on the caller heap, while both keys and values reference the persistent-term entry heap directly. This matches get/1 semantics, since retired entries are retained so previously returned persistent terms remain valid. Add a regression assertion that a persistent_term:get/0 result remains valid after the corresponding entries are erased. Signed-off-by: Peter M <petermm@gmail.com>
1 parent 67707b0 commit 7af87d4

3 files changed

Lines changed: 54 additions & 50 deletions

File tree

‎src/libAtomVM/persistent_term.c‎

Lines changed: 51 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,6 @@
3131
#include "term_hash.h"
3232
#include "utils.h"
3333

34-
#ifndef AVM_NO_SMP
35-
#define SMP_RDLOCK(persistent_term) smp_spinlock_lock(&(persistent_term)->lock)
36-
#define SMP_WRLOCK(persistent_term) smp_spinlock_lock(&(persistent_term)->lock)
37-
#define SMP_UNLOCK(persistent_term) smp_spinlock_unlock(&(persistent_term)->lock)
38-
#else
39-
#define SMP_RDLOCK(persistent_term) UNUSED(persistent_term)
40-
#define SMP_WRLOCK(persistent_term) UNUSED(persistent_term)
41-
#define SMP_UNLOCK(persistent_term) UNUSED(persistent_term)
42-
#endif
43-
4434
struct PersistentTermEntry
4535
{
4636
struct PersistentTermEntry *next;
@@ -52,6 +42,7 @@ struct PersistentTermEntry
5242

5343
static persistent_term_result_t find_entry(
5444
PersistentTerm *persistent_term,
45+
uint32_t bucket_index,
5546
term key,
5647
struct PersistentTermEntry ***out_link,
5748
struct PersistentTermEntry **out_entry,
@@ -71,13 +62,13 @@ void persistent_term_init(PersistentTerm *persistent_term)
7162
}
7263

7364
#ifndef AVM_NO_SMP
74-
smp_spinlock_init(&persistent_term->lock);
65+
persistent_term->lock = smp_rwlock_create();
7566
#endif
7667
}
7768

7869
void persistent_term_destroy(PersistentTerm *persistent_term, GlobalContext *global)
7970
{
80-
SMP_WRLOCK(persistent_term);
71+
SMP_RWLOCK_WRLOCK(persistent_term->lock);
8172
for (size_t i = 0; i < PERSISTENT_TERM_NUM_BUCKETS; i++) {
8273
struct PersistentTermEntry *entry = persistent_term->buckets[i];
8374
while (entry != NULL) {
@@ -97,7 +88,11 @@ void persistent_term_destroy(PersistentTerm *persistent_term, GlobalContext *glo
9788
persistent_term->retired_entries = NULL;
9889
persistent_term->count = 0;
9990
persistent_term->memory = 0;
100-
SMP_UNLOCK(persistent_term);
91+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
92+
#ifndef AVM_NO_SMP
93+
smp_rwlock_destroy(persistent_term->lock);
94+
persistent_term->lock = NULL;
95+
#endif
10196
}
10297

10398
persistent_term_result_t persistent_term_put(
@@ -107,44 +102,48 @@ persistent_term_result_t persistent_term_put(
107102
bool put_new,
108103
GlobalContext *global)
109104
{
110-
SMP_WRLOCK(persistent_term);
105+
uint32_t bucket_index = term_hash(key, global) % PERSISTENT_TERM_NUM_BUCKETS;
106+
107+
struct PersistentTermEntry *new_entry = entry_new(key, value);
108+
if (IS_NULL_PTR(new_entry)) {
109+
return PersistentTermAllocationError;
110+
}
111+
112+
SMP_RWLOCK_WRLOCK(persistent_term->lock);
111113

112114
struct PersistentTermEntry **link;
113115
struct PersistentTermEntry *entry;
114-
persistent_term_result_t result = find_entry(persistent_term, key, &link, &entry, global);
116+
persistent_term_result_t result = find_entry(persistent_term, bucket_index, key, &link, &entry, global);
115117
if (UNLIKELY(result != PersistentTermOk)) {
116-
SMP_UNLOCK(persistent_term);
118+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
119+
entry_destroy(new_entry, global);
117120
return result;
118121
}
119122

120123
if (entry != NULL) {
121124
bool equal = term_is_equal(entry->value, value, global, &result);
122125
if (UNLIKELY(result != PersistentTermOk)) {
123-
SMP_UNLOCK(persistent_term);
126+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
127+
entry_destroy(new_entry, global);
124128
return result;
125129
}
126130

127131
if (equal) {
128-
SMP_UNLOCK(persistent_term);
132+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
133+
entry_destroy(new_entry, global);
129134
return PersistentTermOk;
130135
}
131136

132137
if (put_new) {
133-
SMP_UNLOCK(persistent_term);
138+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
139+
entry_destroy(new_entry, global);
134140
return PersistentTermExists;
135141
}
136142
}
137143

138-
struct PersistentTermEntry *new_entry = entry_new(key, value);
139-
if (IS_NULL_PTR(new_entry)) {
140-
SMP_UNLOCK(persistent_term);
141-
return PersistentTermAllocationError;
142-
}
143-
144144
if (entry == NULL) {
145-
uint32_t idx = term_hash(key, global) % PERSISTENT_TERM_NUM_BUCKETS;
146-
new_entry->next = persistent_term->buckets[idx];
147-
persistent_term->buckets[idx] = new_entry;
145+
new_entry->next = persistent_term->buckets[bucket_index];
146+
persistent_term->buckets[bucket_index] = new_entry;
148147
persistent_term->count++;
149148
persistent_term->memory += new_entry->memory;
150149
} else {
@@ -154,7 +153,7 @@ persistent_term_result_t persistent_term_put(
154153
retire_entry(persistent_term, entry);
155154
}
156155

157-
SMP_UNLOCK(persistent_term);
156+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
158157
return PersistentTermOk;
159158
}
160159

@@ -166,22 +165,24 @@ persistent_term_result_t persistent_term_get(
166165
{
167166
assert(value != NULL);
168167

169-
SMP_RDLOCK(persistent_term);
168+
uint32_t bucket_index = term_hash(key, global) % PERSISTENT_TERM_NUM_BUCKETS;
169+
170+
SMP_RWLOCK_RDLOCK(persistent_term->lock);
170171

171172
struct PersistentTermEntry *entry;
172-
persistent_term_result_t result = find_entry(persistent_term, key, NULL, &entry, global);
173+
persistent_term_result_t result = find_entry(persistent_term, bucket_index, key, NULL, &entry, global);
173174
if (UNLIKELY(result != PersistentTermOk)) {
174-
SMP_UNLOCK(persistent_term);
175+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
175176
return result;
176177
}
177178

178179
if (entry == NULL) {
179-
SMP_UNLOCK(persistent_term);
180+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
180181
return PersistentTermNotFound;
181182
}
182183

183184
*value = entry->value;
184-
SMP_UNLOCK(persistent_term);
185+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
185186
return PersistentTermOk;
186187
}
187188

@@ -195,18 +196,20 @@ persistent_term_result_t persistent_term_erase(
195196

196197
*removed = false;
197198

198-
SMP_WRLOCK(persistent_term);
199+
uint32_t bucket_index = term_hash(key, global) % PERSISTENT_TERM_NUM_BUCKETS;
200+
201+
SMP_RWLOCK_WRLOCK(persistent_term->lock);
199202

200203
struct PersistentTermEntry **link;
201204
struct PersistentTermEntry *entry;
202-
persistent_term_result_t result = find_entry(persistent_term, key, &link, &entry, global);
205+
persistent_term_result_t result = find_entry(persistent_term, bucket_index, key, &link, &entry, global);
203206
if (UNLIKELY(result != PersistentTermOk)) {
204-
SMP_UNLOCK(persistent_term);
207+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
205208
return result;
206209
}
207210

208211
if (entry == NULL) {
209-
SMP_UNLOCK(persistent_term);
212+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
210213
return PersistentTermOk;
211214
}
212215

@@ -215,7 +218,7 @@ persistent_term_result_t persistent_term_erase(
215218
retire_entry(persistent_term, entry);
216219

217220
*removed = true;
218-
SMP_UNLOCK(persistent_term);
221+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
219222
return PersistentTermOk;
220223
}
221224

@@ -226,33 +229,32 @@ persistent_term_result_t persistent_term_get_all_maybe_gc(
226229
{
227230
assert(ret != NULL);
228231

229-
SMP_RDLOCK(persistent_term);
232+
SMP_RWLOCK_RDLOCK(persistent_term->lock);
230233

231234
size_t needed = 0;
232235
for (size_t i = 0; i < PERSISTENT_TERM_NUM_BUCKETS; i++) {
233236
for (struct PersistentTermEntry *entry = persistent_term->buckets[i]; entry != NULL; entry = entry->next) {
234-
needed += CONS_SIZE + TUPLE_SIZE(2) + memory_estimate_usage(entry->key);
237+
needed += CONS_SIZE + TUPLE_SIZE(2);
235238
}
236239
}
237240

238241
if (UNLIKELY(memory_ensure_free_opt(ctx, needed, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {
239-
SMP_UNLOCK(persistent_term);
242+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
240243
return PersistentTermAllocationError;
241244
}
242245

243246
term list = term_nil();
244247
for (size_t i = 0; i < PERSISTENT_TERM_NUM_BUCKETS; i++) {
245248
for (struct PersistentTermEntry *entry = persistent_term->buckets[i]; entry != NULL; entry = entry->next) {
246249
term tuple = term_alloc_tuple(2, &ctx->heap);
247-
term key = memory_copy_term_tree(&ctx->heap, entry->key);
248-
term_put_tuple_element(tuple, 0, key);
250+
term_put_tuple_element(tuple, 0, entry->key);
249251
term_put_tuple_element(tuple, 1, entry->value);
250252
list = term_list_prepend(tuple, list, &ctx->heap);
251253
}
252254
}
253255

254256
*ret = list;
255-
SMP_UNLOCK(persistent_term);
257+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
256258
return PersistentTermOk;
257259
}
258260

@@ -261,14 +263,15 @@ void persistent_term_info(PersistentTerm *persistent_term, size_t *count, size_t
261263
assert(count != NULL);
262264
assert(memory != NULL);
263265

264-
SMP_RDLOCK(persistent_term);
266+
SMP_RWLOCK_RDLOCK(persistent_term->lock);
265267
*count = persistent_term->count;
266268
*memory = persistent_term->memory;
267-
SMP_UNLOCK(persistent_term);
269+
SMP_RWLOCK_UNLOCK(persistent_term->lock);
268270
}
269271

270272
static persistent_term_result_t find_entry(
271273
PersistentTerm *persistent_term,
274+
uint32_t bucket_index,
272275
term key,
273276
struct PersistentTermEntry ***out_link,
274277
struct PersistentTermEntry **out_entry,
@@ -278,8 +281,7 @@ static persistent_term_result_t find_entry(
278281

279282
*out_entry = NULL;
280283

281-
uint32_t idx = term_hash(key, global) % PERSISTENT_TERM_NUM_BUCKETS;
282-
struct PersistentTermEntry **link = &persistent_term->buckets[idx];
284+
struct PersistentTermEntry **link = &persistent_term->buckets[bucket_index];
283285
while (*link != NULL) {
284286
persistent_term_result_t result = PersistentTermOk;
285287
bool equal = term_is_equal((*link)->key, key, global, &result);

‎src/libAtomVM/persistent_term.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ typedef struct PersistentTerm
5252
struct PersistentTermEntry *buckets[PERSISTENT_TERM_NUM_BUCKETS];
5353
struct PersistentTermEntry *retired_entries;
5454
#ifndef AVM_NO_SMP
55-
SpinLock lock;
55+
RWLock *lock;
5656
#endif
5757
} PersistentTerm;
5858

‎tests/erlang_tests/test_persistent_term.erl‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,8 @@ test_info_and_get_all() ->
117117
%% OTP may reclaim that memory immediately.
118118
true = (Memory3 =< Memory2),
119119
true = persistent_term:erase(Key2),
120+
true = lists:member({Key1, {value1, replaced}}, All),
121+
true = lists:member({Key2, {value2, [1, 2, 3]}}, All),
120122
ok.
121123

122124
cleanup() ->

0 commit comments

Comments
 (0)