[PATCH] [merge-tracking] in memory mergeinfo hash for fs_fs

[Kamesh Jayachandran <ka...@collab.net>]

Hi All,
Found a way to cache 'mergeinfo hash' across transactions in memory.
If this patch is accepted may be we can do the same for other diskbased 
persistence in 'bdb'.

With regards
Kamesh Jayachandran


[[[
Patch by: Kamesh Jayachandran <ka...@collab.net>

Currently the 'mergeinfo property' hash is kept in a disk file.
For every mergeinfo property in the commit, we open a file
read the existing hash and update the hash and overwrite the hash again.
As this 'mergeinfo' is not shared across processes, this mergeinfo could be
kept in the memory itself rather than the disk.
Ideally this 'mergeinfo' hash table need to be cached at the transaction 
level.
Unfortunately this is not possible because of the following,
1) fs_change_node_prop do not recieve 'already open txn' as an argument.
2) No way to get the open txn from the 'svn_fs_root_t*'.
3) So 'fs_change_node_prop' creates its own 'txn' and 'commit_body' recieves
   some other open transaction.
As the transactions are different, caching at the txn level will be 
ineffective.

So we are left with caching at the 'fs' level(Even though the txns are 
different,
they both share the same 'fs' pointer).
Ran the merge-tests.py and found the same functioal behaviour before and 
after
this patch, but good speed improvements probably due to in memory 
operations.
Found the proper records in the sqlite database with and without this patch.

single commit (after merge) without the patch
---------------------------------------------
real    0m0.886s
user    0m0.047s
sys     0m0.021s

single commit (after merge) with the patch
---------------------------------------------
real    0m0.282s
user    0m0.046s
sys     0m0.017s

Why I am concerned about this change?
Having the in memory representation will be easy to port the same stuff 
to 'bdb',
or else I need to change the 'schema of transactions' table hold this 
details,
which I don't like to do.


* subversion/libsvn_fs_fs/fs_fs.c
  (PATH_TXN_MERGEINFO): Removed.
  (path_txn_mergeinfo): Removed.
  (get_txn_mergeinfo): Removed.
  (svn_fs_fs__change_txn_mergeinfo):
   Make use of the 'minfo' hash available as a part of the txn->fs.
  (svn_fs_fs__txn_mergeinfo):
   Make use of the 'minfo' hash available as a part of the transaction.
* subversion/libsvn_fs_fs/fs.h
  (struct fs_fs_data_t):
   Adding mergeinfo hash 'minfo'.
]]]

Re: [PATCH] [merge-tracking] in memory mergeinfo hash for fs_fs

[Kamesh Jayachandran <ka...@collab.net>]

Malcolm Rowe wrote:
> On Thu, Aug 31, 2006 at 05:41:03PM +0530, Kamesh Jayachandran wrote:
>   
>> We do this 'mergeinfo' hash cache (forget about in-memory or disk) to 
>> avoid iterating the 'proplist' upon commit to get the same info and not 
>> to rely on 'proplist'(As eventually we won't store mergeinfo as 
>> properties). This 'mergeinfo' hash cache is for our convenience not for 
>> the end user to query, So I feel it does not need to be exposed to disk.
>>
>>     
>
> Ah, so the mergeinfo information is simply a derived version of the
> information already available in the svn:mergeinfo property?  I didn't
> realise that.
>   
Yes.
> As a general question, how do you manage cache coherency between the
> information in the property and the information in the hash?  Or doesn't
> it matter? (perhaps because the property won't change during the period
> that the hash is in use?).
>
>   
They will be coherent as we update the cache hash upon change prop.
>> As This patch caches it at fs level, this info is not available to other 
>> processes like svnadmin which is fine I believe.
>>
>>     
>
> From the sound of things, this cache is an implementation detail, so
> nobody outside of libsvn_fs_fs should need to know about it, or be able
> to detect that it's in use.
>
>   
>> Regarding complication, I am not sure how?
>> Except for the fact it happened to be after thought/round about 
>> solution. The more correct solution would be to make 
>> 'fs_change_node_prop' receive the already open txn.(Which I believe 
>> backward incompatible solution). Then we can use that persistent in 
>> memory txn for caching things like this.
>>     
>
> I don't understand this, I'm afraid.  fs_change_node_prop() takes a
> transaction root, so you already know which transaction it refers to.
>   
No it recieves svn_fs_root_t*, not 'transaction root'.
 From svn_fs_root_t getting the same in memory versions of 'txn' is not 
possible.
All svn_fs_root_t has is 'char *txn' not 'svn_fs_txn_t*'.
Now I think of one solution not sure how correct it is!!!
Why not make 'svn:mergeinfo' as txn prop as against the 'versioned prop' 
and make use of the 'svn_fs_fs__txn_prop' of txn_vtable.
This also solves the duplicate mergeinfo one in sqlite and another in 
properties.

> If you mean that we don't have an fs-internal transaction object to hang
> fs-specific data onto, then, as I understand it, that's a deliberate
> decision [to refer to transactions solely by name inside the FSFS code].
>   
I did not understand this.

With regards
Kamesh Jayachandran

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] [merge-tracking] in memory mergeinfo hash for fs_fs

[Malcolm Rowe <ma...@farside.org.uk>]

On Thu, Aug 31, 2006 at 05:41:03PM +0530, Kamesh Jayachandran wrote:
> We do this 'mergeinfo' hash cache (forget about in-memory or disk) to 
> avoid iterating the 'proplist' upon commit to get the same info and not 
> to rely on 'proplist'(As eventually we won't store mergeinfo as 
> properties). This 'mergeinfo' hash cache is for our convenience not for 
> the end user to query, So I feel it does not need to be exposed to disk.
> 

Ah, so the mergeinfo information is simply a derived version of the
information already available in the svn:mergeinfo property?  I didn't
realise that.

As a general question, how do you manage cache coherency between the
information in the property and the information in the hash?  Or doesn't
it matter? (perhaps because the property won't change during the period
that the hash is in use?).

> As This patch caches it at fs level, this info is not available to other 
> processes like svnadmin which is fine I believe.
> 

Re: [PATCH] [merge-tracking] in memory mergeinfo hash for fs_fs

[Kamesh Jayachandran <ka...@collab.net>]

Malcolm Rowe wrote:
> On Thu, Aug 31, 2006 at 11:32:21AM +0530, Kamesh Jayachandran wrote:
>   
>> Daniel Berlin wrote:
>>     
>>> It's still not going to work.
>>> Again, you can do the following, and it is supposed to work:
>>>
>>> Start an svn transaction in one process
>>> Close the transaction
>>>
>>> Start a new svn process
>>> Reopen the old transaction
>>> Commit it
>>>
>>> If you store it in memory only, this will not work.
>>>       
>> Can you give a usecase where this is needed?
>>     
>
> Dan just did.  Our API explicitly allows the user to do the above,
> and we need to support it, unfortunately.  ('Unfortunately', because
> in my opinion this isn't something that's particularly sensible to do,
> and it complicates the filesystem code).
>   
We do this 'mergeinfo' hash cache (forget about in-memory or disk) to 
avoid iterating the 'proplist' upon commit to get the same info and not 
to rely on 'proplist'(As eventually we won't store mergeinfo as 
properties). This 'mergeinfo' hash cache is for our convenience not for 
the end user to query, So I feel it does not need to be exposed to disk.

As This patch caches it at fs level, this info is not available to other 
processes like svnadmin which is fine I believe.

Regarding complication, I am not sure how?
Except for the fact it happened to be after thought/round about 
solution. The more correct solution would be to make 
'fs_change_node_prop' receive the already open txn.(Which I believe 
backward incompatible solution). Then we can use that persistent in 
memory txn for caching things like this.
>   
>> I remember 'svnadmin lstxns/rmtxns' needs this disk based storage.
>>     
>
> No, you've got it the wrong way round :-).  We need lstxns/rmtxns
> _because_ we support persistent uncommitted transactions.
>   
Not sure what I got wrong?
Yes we need 'svnadmin (ls|rm)txns', not to dump what is there in 
mergeinfo hash cache that we maintain for our internal purposes.

I don't say everything to be in memory, what I say the things like 
'mergeinfo hash' here are the valid candidates for in memory.


With regards
Kamesh Jayachandran
>
> I can't see any reason offhand that you couldn't choose to persist
> the hash to disk only at transaction close time, though.  Mind you,
> that does get into the discussion about the concurrent access that's
> allowed for transactions, though, which we've never really documented.
> (For example, is it valid to open a transaction that's currently opened
> by another thread/process?  It's be good if we could assert that the
> answer was 'no', but I'm not sure whether we can do that).
>
> Regards,
> Malcolm
>
>   

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] [merge-tracking] in memory mergeinfo hash for fs_fs

[Malcolm Rowe <ma...@farside.org.uk>]

On Thu, Aug 31, 2006 at 11:32:21AM +0530, Kamesh Jayachandran wrote:
> Daniel Berlin wrote:
> >It's still not going to work.
> >Again, you can do the following, and it is supposed to work:
> >
> >Start an svn transaction in one process
> >Close the transaction
> >
> >Start a new svn process
> >Reopen the old transaction
> >Commit it
> >
> >If you store it in memory only, this will not work.
> Can you give a usecase where this is needed?

Dan just did.  Our API explicitly allows the user to do the above,
and we need to support it, unfortunately.  ('Unfortunately', because
in my opinion this isn't something that's particularly sensible to do,
and it complicates the filesystem code).

> I remember 'svnadmin lstxns/rmtxns' needs this disk based storage.

No, you've got it the wrong way round :-).  We need lstxns/rmtxns
_because_ we support persistent uncommitted transactions.


I can't see any reason offhand that you couldn't choose to persist
the hash to disk only at transaction close time, though.  Mind you,
that does get into the discussion about the concurrent access that's
allowed for transactions, though, which we've never really documented.
(For example, is it valid to open a transaction that's currently opened
by another thread/process?  It's be good if we could assert that the
answer was 'no', but I'm not sure whether we can do that).

Regards,
Malcolm

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] [merge-tracking] in memory mergeinfo hash for fs_fs

[Kamesh Jayachandran <ka...@collab.net>]

Daniel Berlin wrote:
>> I will dig to the history.
>> With my current patch may be having a hash table of 'txn id'(char*) Vs
>> 'mergeinfo(apr_hash_t*) at the fsap_data should take care of the
>> multiple transaction issue too, as each txn will have its own 'id'.
>>
>> Will evaluate the history and try the above thing and post back to 
>> the list.
>
> It's still not going to work.
> Again, you can do the following, and it is supposed to work:
>
> Start an svn transaction in one process
> Close the transaction
>
> Start a new svn process
> Reopen the old transaction
> Commit it
>
> If you store it in memory only, this will not work.
Can you give a usecase where this is needed?
I remember 'svnadmin lstxns/rmtxns' needs this disk based storage.
I do not think someone would need to see the 'mergeinfo hash' from the 
svnadmin tool, so it can be stored in memory.

With regards
Kamesh Jayachandran
>
>>
>> With regards
>> Kamesh Jayachandran
>>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] [merge-tracking] in memory mergeinfo hash for fs_fs

[Daniel Berlin <db...@dberlin.org>]

> I will dig to the history.
> With my current patch may be having a hash table of 'txn id'(char*) Vs
> 'mergeinfo(apr_hash_t*) at the fsap_data should take care of the
> multiple transaction issue too, as each txn will have its own 'id'.
>
> Will evaluate the history and try the above thing and post back to the list.

It's still not going to work.
Again, you can do the following, and it is supposed to work:

Start an svn transaction in one process
Close the transaction

Start a new svn process
Reopen the old transaction
Commit it

If you store it in memory only, this will not work.

>
> With regards
> Kamesh Jayachandran
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] [merge-tracking] in memory mergeinfo hash for fs_fs

[Kamesh Jayachandran <ka...@collab.net>]

Daniel Berlin wrote:
> On 8/30/06, Kamesh Jayachandran <ka...@collab.net> wrote:
>> Hi All,
>> Found a way to cache 'mergeinfo hash' across transactions in memory.
>
> As i've explained before, this is not going to work.  You have
> absolutely no guarantees that the shared fs pointers are not going to
> be used for different things, like creating two transactions at the
> same time.  If you look at the history of the code, you'll see we did
> this in the past, and then stopped because it is broken.
>
I will dig to the history.
With my current patch may be having a hash table of 'txn id'(char*) Vs 
'mergeinfo(apr_hash_t*) at the fsap_data should take care of the 
multiple transaction issue too, as each txn will have its own 'id'.

Will evaluate the history and try the above thing and post back to the list.

With regards
Kamesh Jayachandran

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] [merge-tracking] in memory mergeinfo hash for fs_fs

[Daniel Berlin <db...@dberlin.org>]

On 8/30/06, Kamesh Jayachandran <ka...@collab.net> wrote:
> Hi All,
> Found a way to cache 'mergeinfo hash' across transactions in memory.

As i've explained before, this is not going to work.  You have
absolutely no guarantees that the shared fs pointers are not going to
be used for different things, like creating two transactions at the
same time.  If you look at the history of the code, you'll see we did
this in the past, and then stopped because it is broken.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org