You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2012/04/26 18:54:26 UTC
Re: svn commit: r1330932 -
/subversion/trunk/subversion/libsvn_fs/fs-loader.c
danielsh@apache.org wrote on Thu, Apr 26, 2012 at 16:30:45 -0000:
> Author: danielsh
> Date: Thu Apr 26 16:30:44 2012
> New Revision: 1330932
>
> URL: http://svn.apache.org/viewvc?rev=1330932&view=rev
> Log:
> Follow-up to r1330906: fix/work-around trail reentrancy assertions in BDB.
>
> (Why is the uuid stored in svn_fs_t->fsap_data rather than in svn_fs_t proper?)
>
> * subversion/libsvn_fs/fs-loader.c
> (cache_uuid): New function.
> (svn_fs_open, svn_fs_create): Call it after everything else.
> (svn_fs_open_berkeley, svn_fs_create_berkeley): Ditto.
The purpose of this is to cause UUID to be cached in BDB's
svn_fs_t->fsap_data so that the new-in-r1330906 calls to
fs->vtable->get_uuid() wouldn't provoke the "run a transaction"
codepath of svn_fs_base__get_uuid().
That seems a bit fragile; I'm not sure whether a better fix should be
found (get the UUID within the existing trail) or whether this should be
documented and svn_fs_base__get_uuid() annotated to point out that the
"run a trail" codepath now only runs once at the time of opening the
svn_fs_t.
Thoughts?
Re: svn commit: r1330932 -
/subversion/trunk/subversion/libsvn_fs/fs-loader.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Thu, Apr 26, 2012 at 14:01:24 -0400:
> On Apr 26, 2012 1:56 PM, "Daniel Shahaf" <da...@elego.de> wrote:
> >
> > Greg Stein wrote on Thu, Apr 26, 2012 at 13:29:55 -0400:
> > > On Apr 26, 2012 12:55 PM, "Daniel Shahaf" <da...@elego.de> wrote:
> > > >
> > > > danielsh@apache.org wrote on Thu, Apr 26, 2012 at 16:30:45 -0000:
> > > > > Author: danielsh
> > > > > Date: Thu Apr 26 16:30:44 2012
> > > > > New Revision: 1330932
> > > > >
> > > > > URL: http://svn.apache.org/viewvc?rev=1330932&view=rev
> > > > > Log:
> > > > > Follow-up to r1330906: fix/work-around trail reentrancy assertions
> in
> > > BDB.
> > > > >
> > > > > (Why is the uuid stored in svn_fs_t->fsap_data rather than in
> svn_fs_t
> > > proper?)
> > > > >
> > > > > * subversion/libsvn_fs/fs-loader.c
> > > > > (cache_uuid): New function.
> > > > > (svn_fs_open, svn_fs_create): Call it after everything else.
> > > > > (svn_fs_open_berkeley, svn_fs_create_berkeley): Ditto.
> > > >
> > > > The purpose of this is to cause UUID to be cached in BDB's
> > > > svn_fs_t->fsap_data so that the new-in-r1330906 calls to
> > > > fs->vtable->get_uuid() wouldn't provoke the "run a transaction"
> > > > codepath of svn_fs_base__get_uuid().
> > > >
> > > > That seems a bit fragile; I'm not sure whether a better fix should be
> > > > found (get the UUID within the existing trail) or whether this should
> be
> > > > documented and svn_fs_base__get_uuid() annotated to point out that the
> > > > "run a trail" codepath now only runs once at the time of opening the
> > > > svn_fs_t.
> > > >
> > > > Thoughts?
> > >
> > > Put the uuid into svn_fs_t and fill it at creation time using the
> vtable.
> > > Change svn_fs_get_uuid to return that, rather than using the vtable.
> > >
> >
> > And delete the fs_vtable_t->get_uuid and fs_vtable_t->set_uuid callbacks
> > altogether, I presume.
>
> Umm. No... don't you still need the vtable to access/set the persisted
> value?
>
No, I don't. svn_fs_set_uuid() can set a hypothetical svn_fs_t->uuid
field itself; it only needs a vtable->set_uuid() helper if it wants to
set fsap_data->uuid (which exists in both backends, but fsap_data is
opaque to libsvn_fs).
> I'm just suggesting that the vtable is used at init time, rather than every
> call to get_uuid.
>
Where do you envision the UUID stored/cached --- in svn_fs_t or in
svn_fs_t->fsap_data? I understand we move it from the latter to the
former.
> Cheers,
> -g
Re: svn commit: r1330932 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c
Posted by Greg Stein <gs...@gmail.com>.
On Apr 26, 2012 1:56 PM, "Daniel Shahaf" <da...@elego.de> wrote:
>
> Greg Stein wrote on Thu, Apr 26, 2012 at 13:29:55 -0400:
> > On Apr 26, 2012 12:55 PM, "Daniel Shahaf" <da...@elego.de> wrote:
> > >
> > > danielsh@apache.org wrote on Thu, Apr 26, 2012 at 16:30:45 -0000:
> > > > Author: danielsh
> > > > Date: Thu Apr 26 16:30:44 2012
> > > > New Revision: 1330932
> > > >
> > > > URL: http://svn.apache.org/viewvc?rev=1330932&view=rev
> > > > Log:
> > > > Follow-up to r1330906: fix/work-around trail reentrancy assertions
in
> > BDB.
> > > >
> > > > (Why is the uuid stored in svn_fs_t->fsap_data rather than in
svn_fs_t
> > proper?)
> > > >
> > > > * subversion/libsvn_fs/fs-loader.c
> > > > (cache_uuid): New function.
> > > > (svn_fs_open, svn_fs_create): Call it after everything else.
> > > > (svn_fs_open_berkeley, svn_fs_create_berkeley): Ditto.
> > >
> > > The purpose of this is to cause UUID to be cached in BDB's
> > > svn_fs_t->fsap_data so that the new-in-r1330906 calls to
> > > fs->vtable->get_uuid() wouldn't provoke the "run a transaction"
> > > codepath of svn_fs_base__get_uuid().
> > >
> > > That seems a bit fragile; I'm not sure whether a better fix should be
> > > found (get the UUID within the existing trail) or whether this should
be
> > > documented and svn_fs_base__get_uuid() annotated to point out that the
> > > "run a trail" codepath now only runs once at the time of opening the
> > > svn_fs_t.
> > >
> > > Thoughts?
> >
> > Put the uuid into svn_fs_t and fill it at creation time using the
vtable.
> > Change svn_fs_get_uuid to return that, rather than using the vtable.
> >
>
> And delete the fs_vtable_t->get_uuid and fs_vtable_t->set_uuid callbacks
> altogether, I presume.
Umm. No... don't you still need the vtable to access/set the persisted
value?
I'm just suggesting that the vtable is used at init time, rather than every
call to get_uuid.
Cheers,
-g
Re: svn commit: r1330932 -
/subversion/trunk/subversion/libsvn_fs/fs-loader.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Thu, Apr 26, 2012 at 13:29:55 -0400:
> On Apr 26, 2012 12:55 PM, "Daniel Shahaf" <da...@elego.de> wrote:
> >
> > danielsh@apache.org wrote on Thu, Apr 26, 2012 at 16:30:45 -0000:
> > > Author: danielsh
> > > Date: Thu Apr 26 16:30:44 2012
> > > New Revision: 1330932
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1330932&view=rev
> > > Log:
> > > Follow-up to r1330906: fix/work-around trail reentrancy assertions in
> BDB.
> > >
> > > (Why is the uuid stored in svn_fs_t->fsap_data rather than in svn_fs_t
> proper?)
> > >
> > > * subversion/libsvn_fs/fs-loader.c
> > > (cache_uuid): New function.
> > > (svn_fs_open, svn_fs_create): Call it after everything else.
> > > (svn_fs_open_berkeley, svn_fs_create_berkeley): Ditto.
> >
> > The purpose of this is to cause UUID to be cached in BDB's
> > svn_fs_t->fsap_data so that the new-in-r1330906 calls to
> > fs->vtable->get_uuid() wouldn't provoke the "run a transaction"
> > codepath of svn_fs_base__get_uuid().
> >
> > That seems a bit fragile; I'm not sure whether a better fix should be
> > found (get the UUID within the existing trail) or whether this should be
> > documented and svn_fs_base__get_uuid() annotated to point out that the
> > "run a trail" codepath now only runs once at the time of opening the
> > svn_fs_t.
> >
> > Thoughts?
>
> Put the uuid into svn_fs_t and fill it at creation time using the vtable.
> Change svn_fs_get_uuid to return that, rather than using the vtable.
>
And delete the fs_vtable_t->get_uuid and fs_vtable_t->set_uuid callbacks
altogether, I presume.
> If there is a way to change it, then do the right magic to reset the value
> in svn_fs_t, as appropriate.
>
> Cheers,
> -g
THanks
Daniel
Re: svn commit: r1330932 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c
Posted by Greg Stein <gs...@gmail.com>.
On Apr 26, 2012 12:55 PM, "Daniel Shahaf" <da...@elego.de> wrote:
>
> danielsh@apache.org wrote on Thu, Apr 26, 2012 at 16:30:45 -0000:
> > Author: danielsh
> > Date: Thu Apr 26 16:30:44 2012
> > New Revision: 1330932
> >
> > URL: http://svn.apache.org/viewvc?rev=1330932&view=rev
> > Log:
> > Follow-up to r1330906: fix/work-around trail reentrancy assertions in
BDB.
> >
> > (Why is the uuid stored in svn_fs_t->fsap_data rather than in svn_fs_t
proper?)
> >
> > * subversion/libsvn_fs/fs-loader.c
> > (cache_uuid): New function.
> > (svn_fs_open, svn_fs_create): Call it after everything else.
> > (svn_fs_open_berkeley, svn_fs_create_berkeley): Ditto.
>
> The purpose of this is to cause UUID to be cached in BDB's
> svn_fs_t->fsap_data so that the new-in-r1330906 calls to
> fs->vtable->get_uuid() wouldn't provoke the "run a transaction"
> codepath of svn_fs_base__get_uuid().
>
> That seems a bit fragile; I'm not sure whether a better fix should be
> found (get the UUID within the existing trail) or whether this should be
> documented and svn_fs_base__get_uuid() annotated to point out that the
> "run a trail" codepath now only runs once at the time of opening the
> svn_fs_t.
>
> Thoughts?
Put the uuid into svn_fs_t and fill it at creation time using the vtable.
Change svn_fs_get_uuid to return that, rather than using the vtable.
If there is a way to change it, then do the right magic to reset the value
in svn_fs_t, as appropriate.
Cheers,
-g