libunwind-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Milian Wolff
Subject: Re: [Libunwind-devel] WIP patch: configurable cache size
Date: Sat, 04 Oct 2014 19:19:23 +0200
User-agent: KMail/4.14 (Linux/3.16.3-1-ARCH; KDE/4.14.0; x86_64; ; )

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

> * 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?

> * > 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.

> Looking forward to the full patch.

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

Attachment: configurable_cache_size.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]