[DTrace-devel] [PATCH 1/7] htab reduction: kernpath

[Nick Alcock]

On 8 Oct 2021, Eugene Loh uttered the following:

> One other thing.  (All this falls under "I'm curious now that I'm 
> reading this stuff"... not "I am outraged and you need to make changes.")

:)

> On 10/5/21 9:38 AM, Nick Alcock wrote:
>> diff --git a/libdtrace/dt_kernel_module.c b/libdtrace/dt_kernel_module.c
>> @@ -29,6 +29,34 @@
>> +static dt_kern_path_t *
>> +dt_kern_path_lookup(dtrace_hdl_t *dtp, const char *name)
>> +{
>> +	dt_kern_path_t tmpl;
>> +
>> +	if (_dt_unlikely_(!dtp->dt_kernpaths))
>> +		return NULL;
>> +
>> +	tmpl.dkp_name = (char *) name;
>> +	return dt_htab_lookup(dtp->dt_kernpaths, &tmpl);
>> +}
> This short function can perhaps simply be inlined into its two call 
> sites.  I mean, why both lookup(dtp,name) and lookup_by_name(dtp,name)?

Good point! I was keeping it around because it was *just* long enough to
be worth it... but with the check gone, it's not, and the check is only
there because I didn't check the callers... adjusted.

> The code to be inlined is even shorter if the "if(!dtp->dt_kernpaths)" 
> check is eliminated, which makes sense given that the callers already 
> check.  Further, why do we even check?  If dt_htab_lookup(NULL, foo) is 
> called, it segfaults. Wouldn't it make more sense for dt_htab_lookup() 
> to check for htab==NULL and return NULL in that case?
>
> I'm just asking.  I'm still trying to understand what a coherent use of 
> htabs would look like.

Usually, allocate, assume present when using, destroy with all members
via a destructor in the same conceptual entity that did the allocation:
not, as I did here, allocate implicitly, destroy once empty, equally
implicitly.

... but kernpaths are a little special, since we *do* want to allocate
them only lazily when dt_kern_path_lookup_by_name is called, because it
is only rarely called these days and is distinctly expensive to
construct. But still, we want to deallocate via a single
dt_htab_destroy(dtp->dt_kernpaths), not a loop over a list we never use
for anything else.)

New push coming up soon that does that, once everything else is adjusted
too :)

-- 
NULL && (void)