RE: svn commit: r1203344 - /subversion/trunk/subversion/libsvn_delta/compat.c

[Bert Huijben <be...@qqmail.nl>]

> -----Original Message-----
> From: hwright@apache.org [mailto:hwright@apache.org]
> Sent: donderdag 17 november 2011 20:37
> To: commits@subversion.apache.org
> Subject: svn commit: r1203344 -
> /subversion/trunk/subversion/libsvn_delta/compat.c
> 
> Author: hwright
> Date: Thu Nov 17 19:37:29 2011
> New Revision: 1203344
> 
> URL: http://svn.apache.org/viewvc?rev=1203344&view=rev
> Log:
> Ev2 shims: Properly pass around the target rev value.  Since Ev2 doesn't have
> this concept (similar to "start of edit") we have to pass this value through
> a secondary callback.  Ugly.

How did this happen? Did we miss this in the design?

The update editor uses the target revision in every changing callback (new revision) and some of the other callbacks (tree conflicts, etc.). When a node is changed by an update it is brought in the state of that revision, so that revision should be well known. Especially because we bump the revision of *all* nodes that aren't touched by an update after the update.


Seeing your progress:
How well is editor v2 going to perform in the for the next few years after release common scenarios where either a client or the repository is not going to be using editor v2. 

The number of added callbacks that weren't in the initial plan to retrieve information from the working copy are making me nervous. Every single call in to wc.db has a non-zero cost to obtain and release the read lock. 

I had to remove quite a few db calls to make 1.7 perform as good on updating a node as it does now and it could really use more improvement.


"If our editor v2 would perform 3* slower than the current update process on the currently installed servers, it isn't going to work for our users."


Of course our future performance is just guessing now, but maybe we should verify if our design still covers our needs.


	Bert 

Re: svn commit: r1203344 - /subversion/trunk/subversion/libsvn_delta/compat.c

[Greg Stein <gs...@gmail.com>]

On Fri, Nov 18, 2011 at 04:05, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: hwright@apache.org [mailto:hwright@apache.org]
>> Sent: donderdag 17 november 2011 20:37
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1203344 -
>> /subversion/trunk/subversion/libsvn_delta/compat.c
>>
>> Author: hwright
>> Date: Thu Nov 17 19:37:29 2011
>> New Revision: 1203344
>>
>> URL: http://svn.apache.org/viewvc?rev=1203344&view=rev
>> Log:
>> Ev2 shims: Properly pass around the target rev value.  Since Ev2 doesn't have
>> this concept (similar to "start of edit") we have to pass this value through
>> a secondary callback.  Ugly.
>
> How did this happen? Did we miss this in the design?

Not all editor drives know a "target" revision. Consider a commit
drive. Or how about the editor drive that corresponds to "svn status
-u".

It is really just an "update" drive that results in modifying the
target tree's revision. That target should be described at the time
the Ev2 editor is constructed. "Give me an editor that will move the
target to revision R."

There is no problem in the *design*, but merely in the use of *shims*.
When set_target_revision is called, it should store that into the
editor state. When complete is called, the tree should be bumped. BUT:
since the editor state/baton is managed by the shims, Hyrum needs a
callback to transfer a potential target revision to the shim owner. A
separate shim will then use that value to inject it into a delta
editor's set_target_revision.

In short: these are aspects of the *shims*, and more particularly the
paired-shims.

A proper Ev2 update drive would construct the editor once the target
revision is known, and that constructor would have a target_rev
parameter, which would then be stored in the editor's private state.
At complete() time, that editor would do the bump of all nodes.

The *shims* have no mechanism to do a bump. I don't even know how this
callback will help with that. Hyrum probably has some plans for that.
I would think there should be a callback that is called by the shim's
implementation of complete() (call it "bump_func"), that is called
with a target_rev saved via set_target_revision. Just saving the rev
seems to fine-grained, given the semantics of why that is ever called.

>...
> The number of added callbacks that weren't in the initial plan to retrieve information from the working copy are making me nervous. Every single call in to wc.db has a non-zero cost to obtain and release the read lock.

The callbacks aren't an artifact of a lack in the Ev2 interface, but a
result of using generic shims.

>...

Cheers,
-g