On Mon, 22 Jan 2007, Peter Lundblad wrote:
> Kamesh Jayachandran writes:
> > Peter Lundblad wrote:
> > > I am not sure I find this way "better". Sure, it does avoid a slight
> > > allocation out of a pool. OTOH, when one reads this cod,e one has to
> > > think "hey, where does this hash table come from, is it considered
> > > read-only, and am I sure no caller up the call chain doesn't pass it
> > > to multiple threads at a time?" Is this microoptimization really worth
> > >
> > Out of 5 instances where this patch changes, 3 are straight forward
> > local hash tables. And no new threads could have access to this local
> > pointers.
>
> I am not disputing that fact. My point is that when reviewing this code,
> you have to make sure that's the case, and what does it actually buy us?
>
> > We already do this at 'determine_merges_performed' like
> > 'apr_hash_first(NULL, notify_b->skipped_paths)'.
>
> Sorry for not paying attention then, but I'll also call this a premature
> optimization. I'd rather like us be consistent here since we almost are.
> Show me that it can make a real difference, and I'll of course reconsider;)
I doubt it buys us much in terms of memory footprint, but examing the
flip side of the coin, what does it cost us? For local variables, I
find the complexity cost to be negliable. (For non-local variables, I
wouldn't favor this approach.) One could argue that this difference
is in itself complexity cost, but for such a widely used API as that
of apr_hash_t, I don't buy that argument. As a developer, you simply
have to become familiar with an API used so often throughout both
Subversion's interface and implementation.
- application/pgp-signature attachment: stored
Received on Mon Jan 22 22:34:51 2007