Make arenas_lookup_ctl triable #2424
Conversation
| arena_t *arena; | ||
|
|
||
| ptr = NULL; | ||
| ret = EINVAL; | ||
| malloc_mutex_lock(tsd_tsdn(tsd), &ctl_mtx); | ||
| WRITE(ptr, void *); | ||
| edata = emap_edata_lookup(tsd_tsdn(tsd), &arena_emap_global, ptr); | ||
| if (edata == NULL) { | ||
| ptr_not_present = emap_full_alloc_ctx_try_lookup(tsd_tsdn(tsd), &arena_emap_global, ptr, |
There was a problem hiding this comment.
This sounds like a good idea given that we got rid of ivsalloc. I'll wait a bit to merge in case @guangli-dai or @davidtgoldblatt have any concerns.
One thing to note is that emap_full_alloc_ctx_try_lookup is slightly slower since it cannot assume the lookup won't fail, so has a few more branches down the path.
Another way to make the entire call more efficient from the application side, is to do mallctlnametomib when initialize, and call mallctlbymib afterwards, instead of doing mallctl every time (which walks the namespace tree).
There was a problem hiding this comment.
maybe calling rtree_read_independent here is better?
There was a problem hiding this comment.
This looks good to me. If we are worried about the efficiency of using emap_full_alloc_ctx_try_lookup here (although I don't think it should bother anyone), we can create another function, i.e., arenas_lookup_triable_ctl, which allows a triable lookup and keep arenas_lookup_ctl as it was in case anyone is using it in any efficiency-critical scenarios?
There was a problem hiding this comment.
I think from the pure user side, calling mallctl("arenas.lookup", ...) is expecting just lookup and triable. It may not be good to add arenas_lookup_triable_ctl here.
There was a problem hiding this comment.
Yeah I'm hoping we don't have to add another API specifically for the triage. After all the added overhead is a few branches, so only would be noticeable if calling very frequently (e.g. millions per second). Also there's one trick we have not done on the rtree look up path -- for potentially fail-able lookups, we can make all invalid / null nodes in the rtree point to a special node (which then points to itself) to indicate look up failure. That way the lookup will always go through the fixed levels and only need to check for failure last. Since the frequent lookups are all dependent ones, that optimization hasn't been necessary yet.
maybe calling rtree_read_independent here is better?
No need to change really. rtree accesses should go through the emap (which is a wrapper in this case) anyway.
I'll merge in a couple of days if no other concerns here.
|
Thanks again for the patch and discussion @auxten ! |
See #2425