You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2006/10/06 19:18:30 UTC
[PATCH] why not loop through apr_hash_t with pool as NULL.
Hi All,
Found a snippet of code in subversion/libsvn_fs_base/tree.c where we
create a separate subpool which is passed to apr_hash_first alone. And
this subpool can be replaced by NULL.
With this change I ran 'make check FS_TYPE=bdb' and observed same
behaviour without this change.
Can someone consider this patch?
With regards
Kamesh Jayachandran
Re: [PATCH] why not loop through apr_hash_t with pool as NULL.
Posted by Daniel Rall <dl...@collab.net>.
Use of a NULL pool with apr_hash_first() is fine so long as your
apr_hash_t is only accessed by one thread at a time. Is that the case
here?
On Sat, 07 Oct 2006, Kamesh Jayachandran wrote:
> Hi All,
> Found a snippet of code in subversion/libsvn_fs_base/tree.c where we
> create a separate subpool which is passed to apr_hash_first alone. And
> this subpool can be replaced by NULL.
> With this change I ran 'make check FS_TYPE=bdb' and observed same
> behaviour without this change.
>
> Can someone consider this patch?
>
> With regards
> Kamesh Jayachandran
> [[[
> Patch by: Kamesh Jayachandran <ka...@collab.net>
>
> Why to create subpool to iterate the apr_hash_t rather iterate with
> a NULL as pool.
>
> * subversion/libsvn_fs_base/tree.c
> (base_dir_entries):
> Don't create subpool to iterate over the apr_hash_t rather pass NULL as
> pool to apr_hash_first.
>
> ]]]
> Index: subversion/libsvn_fs_base/tree.c
> ===================================================================
> --- subversion/libsvn_fs_base/tree.c (revision 21795)
> +++ subversion/libsvn_fs_base/tree.c (working copy)
> @@ -1465,8 +1465,7 @@
> if (table)
> {
> apr_hash_index_t *hi;
> - apr_pool_t *subpool = svn_pool_create(pool);
> - for (hi = apr_hash_first(subpool, table); hi; hi = apr_hash_next(hi))
> + for (hi = apr_hash_first(NULL, table); hi; hi = apr_hash_next(hi))
> {
> svn_fs_dirent_t *entry;
> struct node_kind_args nk_args;
Re: [PATCH] why not loop through apr_hash_t with pool as NULL.
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 06 Oct 2006, Philip Martin wrote:
> Daniel Rall <dl...@collab.net> writes:
...
> > /* KEY will be the entry name in ancestor (about which we
> > simple don't care), VAL the dirent. */
> > apr_hash_this(hi, NULL, NULL, &val);
> > entry = val;
> > nk_args.id = entry->id;
> > SVN_ERR(svn_fs_base__retry_txn(fs, txn_body_node_kind, &nk_args,
> > - pool));
> > + subpool));
> > entry->kind = nk_args.kind;
> > }
> > + svn_pool_destroy(subpool);
> > }
> > else
> > {
> >
> > This may not provide meaningful memory savings -- how expensive is
> > svn_fs_base__retry_txn()?
>
> Not expensive enough for anyone to notice so far :)
:-)
I've removed use of the sub-pool in r21818.
Re: [PATCH] why not loop through apr_hash_t with pool as NULL.
Posted by Philip Martin <ph...@codematters.co.uk>.
Daniel Rall <dl...@collab.net> writes:
>
> + svn_pool_clear(subpool);
> +
The iterator is allocated from subpool, it would need to use pool if
subpool is cleared.
> /* KEY will be the entry name in ancestor (about which we
> simple don't care), VAL the dirent. */
> apr_hash_this(hi, NULL, NULL, &val);
> entry = val;
> nk_args.id = entry->id;
> SVN_ERR(svn_fs_base__retry_txn(fs, txn_body_node_kind, &nk_args,
> - pool));
> + subpool));
> entry->kind = nk_args.kind;
> }
> + svn_pool_destroy(subpool);
> }
> else
> {
>
> This may not provide meaningful memory savings -- how expensive is
> svn_fs_base__retry_txn()?
Not expensive enough for anyone to notice so far :)
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] why not loop through apr_hash_t with pool as NULL.
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 06 Oct 2006, Philip Martin wrote:
...
> > Index: subversion/libsvn_fs_base/tree.c
> > ===================================================================
> > --- subversion/libsvn_fs_base/tree.c (revision 21795)
> > +++ subversion/libsvn_fs_base/tree.c (working copy)
> > @@ -1465,8 +1465,7 @@
> > if (table)
> > {
> > apr_hash_index_t *hi;
> > - apr_pool_t *subpool = svn_pool_create(pool);
> > - for (hi = apr_hash_first(subpool, table); hi; hi = apr_hash_next(hi))
> > + for (hi = apr_hash_first(NULL, table); hi; hi = apr_hash_next(hi))
> > {
> > svn_fs_dirent_t *entry;
> > struct node_kind_args nk_args;
>
> The way the subpool is used looks wrong. Why is the subpool used at
> all? Surely the overhead for a single iterator is small enough to
> come out of pool.
>
> If the subpool is required why is it not cleared and destroyed?
There does not appear to be a great need for a sub-pool here, but it
could be leveraged inside the loop:
Index: subversion/libsvn_fs_base/tree.c
===================================================================
--- subversion/libsvn_fs_base/tree.c (revision 21813)
+++ subversion/libsvn_fs_base/tree.c (working copy)
@@ -1472,15 +1472,18 @@
struct node_kind_args nk_args;
void *val;
+ svn_pool_clear(subpool);
+
/* KEY will be the entry name in ancestor (about which we
simple don't care), VAL the dirent. */
apr_hash_this(hi, NULL, NULL, &val);
entry = val;
nk_args.id = entry->id;
SVN_ERR(svn_fs_base__retry_txn(fs, txn_body_node_kind, &nk_args,
- pool));
+ subpool));
entry->kind = nk_args.kind;
}
+ svn_pool_destroy(subpool);
}
else
{
This may not provide meaningful memory savings -- how expensive is
svn_fs_base__retry_txn()?
This code was injected as part of a FSFS-related branch merge:
------------------------------------------------------------------------
r9572 | ghudson | 2004-04-29 16:17:29 -0700 (Thu, 29 Apr 2004) | 4 lines
Merge the changes from the fs-abstraction branch. This allows the
libsvn_fs_fs filesystem to sit alongside the old BDB filesystem in the
same executable.
------------------------------------------------------------------------
It may've been cribbed from surrounding BDB code in a following
function which has a similar structure.
Re: [PATCH] why not loop through apr_hash_t with pool as NULL.
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 10/6/06, Philip Martin <ph...@codematters.co.uk> wrote:
> "Garrett Rooney" <ro...@electricjellyfish.net> writes:
>
> > Is that hash table actually accessed from multiple threads? The vast
> > majority of our code consists of data structures that are not shared
> > among more than oen thread...
>
> Good point. On the other hand, given that it's only a single
> iterator, simply using pool avoids the question now and in the future.
True, assuming we have a pool available it's almost certainly the
right thing to do, since it's an infintesimally small amount of memory
to use, and at the very least it keeps someone from copying this code
into someplace where thread safety does matter and screwing things up.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] why not loop through apr_hash_t with pool as NULL.
Posted by Philip Martin <ph...@codematters.co.uk>.
"Garrett Rooney" <ro...@electricjellyfish.net> writes:
> Is that hash table actually accessed from multiple threads? The vast
> majority of our code consists of data structures that are not shared
> among more than oen thread...
Good point. On the other hand, given that it's only a single
iterator, simply using pool avoids the question now and in the future.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] why not loop through apr_hash_t with pool as NULL.
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 10/6/06, Philip Martin <ph...@codematters.co.uk> wrote:
> Kamesh Jayachandran <ka...@collab.net> writes:
>
> > Found a snippet of code in subversion/libsvn_fs_base/tree.c where we
> > create a separate subpool which is passed to apr_hash_first alone. And
> > this subpool can be replaced by NULL.
>
> Not thread safe.
Is that hash table actually accessed from multiple threads? The vast
majority of our code consists of data structures that are not shared
among more than oen thread...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] why not loop through apr_hash_t with pool as NULL.
Posted by Philip Martin <ph...@codematters.co.uk>.
Kamesh Jayachandran <ka...@collab.net> writes:
> Found a snippet of code in subversion/libsvn_fs_base/tree.c where we
> create a separate subpool which is passed to apr_hash_first alone. And
> this subpool can be replaced by NULL.
Not thread safe.
> Index: subversion/libsvn_fs_base/tree.c
> ===================================================================
> --- subversion/libsvn_fs_base/tree.c (revision 21795)
> +++ subversion/libsvn_fs_base/tree.c (working copy)
> @@ -1465,8 +1465,7 @@
> if (table)
> {
> apr_hash_index_t *hi;
> - apr_pool_t *subpool = svn_pool_create(pool);
> - for (hi = apr_hash_first(subpool, table); hi; hi = apr_hash_next(hi))
> + for (hi = apr_hash_first(NULL, table); hi; hi = apr_hash_next(hi))
> {
> svn_fs_dirent_t *entry;
> struct node_kind_args nk_args;
The way the subpool is used looks wrong. Why is the subpool used at
all? Surely the overhead for a single iterator is small enough to
come out of pool. If the subpool is required why is it not cleared
and destroyed?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org