Re: [Libunwind-devel] WIP patch: configurable cache size

[Milian Wolff]

On Saturday 04 October 2014 19:19:23 Milian Wolff wrote:
> On Saturday 04 October 2014 08:34:49 Arun Sharma wrote:
> > On Sat, Oct 4, 2014 at 7:04 AM, Milian Wolff <address@hidden> wrote:
> > > Hey all,
> > > 
> > > attached you find the current state of my proof-of-concept patch for a
> > > configurable cache size. I'd like to get early feedback if possible, as
> > > I'm
> > > very new to libunwind's codebase and coding style. Also, I usually write
> > > C++ code, so I more or less just copied other C code and may have
> > > misunderstood a few things here and there. The patch is also missing a
> > > proper test and documentation, something I'll try to remedy in the
> > > coming
> > > days.
> > 
> > GNU coding style isn't my usual coding style either :) Let's worry
> > about the logic/interface first.
> > Your changes look reasonable, some comments below:
> > 
> > * {L,G}set_cache_log_size.c - I didn't see this file in the patch.
> 
> Doh, see attached patch which includes these files.
> 
> > * Does it make sense to add a new file or just add it to the existing
> > *caching_policy.c?
> 
> That's what I deduced from the existence of *caching_policy.c with just one
> function (and the file is named after the function). I'm also fine with
> adding the methods there if that's OK for you. I'll do that in the
> follow-up commit then

I've merged this locally now.

> > * global vs thread local vs none should be based on the caching
> > policy. It's possible that this is what you've done.
> 
> That is actually something I'm not so sure about I did correctly. If I'm not
> mistaken, the old cache behavior was always global, no? It always used the
> cache set in the unw_addr_space_t (which is global, and not per-thread,
> no?). Now, with the code I copied over, I think it's going to be
> global/thread- local/none as you say. But if someone could review that, I'd
> appreciate that.
>
> > * s/unthreaded/global/
> 
> OK, note that most of this code comes from src/x86_64/Gtrace.c which uses
> the "unthreaded" nomenclature. Generally, is there a way in C to share
> some/most of this code somehow? In C++, I'd use a template, in C - what do
> I do? Copy'n'paste the stuff as it's done now, or macrofy more of this and
> share the code that way?

Ping, any input on the two above points?

> > * > unw_set_cache_log_size - why not unw_set_cache_size() and then
> > round up to the nearest power of 2?
> 
> Can do if you prefer that.

I wanted to implement this now and realized that it will probably complicate 
the code quite a lot:

If I round up to the nearest power of 2, what should the "size" mean? Bytes? 
Right now the whole code works with the logarithmic size that indicates the 
bucket size. If we want the user to supply a byte-size it will be tricky (no?) 
to convert that to a meaningful log size for the code where N items fit in.

If we just take the input number as number of items in the cache, I'd say we 
could directly ask for the cache size as a logarithmic number, no? This 
function will be seldom used after all. As a user of libunwind, I personally 
couldn't care less whether I have to write

unw_set_cache_log_size(10)

or

unw_set_cache_size(1024)

What do you say, should I still refactor the code to take in a byte size from 
the user?

> > Looking forward to the full patch.

With more input, I'll also provide a follow-up patch.

Bye

-- 
Milian Wolff
address@hidden
http://milianw.de