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 <d....@daniel.shahaf.name> on 2015/03/29 09:44:40 UTC
Re: svn commit: r1669743 -
/subversion/trunk/subversion/libsvn_fs_fs/transaction.c
stefan2@apache.org wrote on Sat, Mar 28, 2015 at 10:58:13 -0000:
> Author: stefan2
> Date: Sat Mar 28 10:58:13 2015
> New Revision: 1669743
>
> URL: http://svn.apache.org/r1669743
> Log:
> Follow-up to r1654934:
> Relax the size check in the FSFS representation sharing code to make it as
> effective as in earlier releases.
>
> The size check has been to strict and would not allow two matching reps to
> have a different base representation, e.g. mod on /trunk and add-without-
> history on a branch. Mainly depending on the merge policy (catch-up vs.
> cherry-pick), this resulted in detecting and eliding fewer instances of
> redundant representations. Hence, larger repositories.
>
> * subversion/libsvn_fs_fs/transaction.c
> (get_shared_rep): Care about the on-disk size only if the fulltext /
> expanded size is not known - which would be the case
> for e.g. PLAIN representations.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/transaction.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1669743&r1=1669742&r2=1669743&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Sat Mar 28 10:58:13 2015
> @@ -2214,10 +2214,11 @@ get_shared_rep(representation_t **old_re
> return SVN_NO_ERROR;
>
> /* We don't want 0-length PLAIN representations to replace non-0-length
> - ones (see issue #4554). Also, this doubles as a simple guard against
> - general rep-cache induced corruption. */
> + ones (see issue #4554). Take into account that EXPANDED_SIZE may be
> + 0 in which case we have to check the on-disk SIZE. Also, this doubles
> + as a simple guard against general rep-cache induced corruption. */
> if ( ((*old_rep)->expanded_size != rep->expanded_size)
> - || ((*old_rep)->size != rep->size))
> + || (rep->expanded_size && ((*old_rep)->size != rep->size)))
If two reps have EXPANDED_SIZE == 0 and different SIZE, the condition
will be FALSE. Shouldn't it be TRUE instead?
When comparing SIZE, shouldn't it also compare the rep kind (PLAIN or
DELTA) at the same time? Otherwise, two reps that have the same
EXPANDED_SIZE, different SIZE, and different kinds wouldn't be shared,
even though they could be.
Daniel
> {
> *old_rep = NULL;
> }
>
>
Re: svn commit: r1669743 - /subversion/trunk/subversion/libsvn_fs_fs/transaction.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Mar 29, 2015 at 9:44 AM, Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> stefan2@apache.org wrote on Sat, Mar 28, 2015 at 10:58:13 -0000:
> > Author: stefan2
> > Date: Sat Mar 28 10:58:13 2015
> > New Revision: 1669743
> >
> > URL: http://svn.apache.org/r1669743
> > Log:
> > Follow-up to r1654934:
> > Relax the size check in the FSFS representation sharing code to make it
> as
> > effective as in earlier releases.
> >
> > The size check has been to strict and would not allow two matching reps
> to
> > have a different base representation, e.g. mod on /trunk and add-without-
> > history on a branch. Mainly depending on the merge policy (catch-up vs.
> > cherry-pick), this resulted in detecting and eliding fewer instances of
> > redundant representations. Hence, larger repositories.
> >
> > * subversion/libsvn_fs_fs/transaction.c
> > (get_shared_rep): Care about the on-disk size only if the fulltext /
> > expanded size is not known - which would be the case
> > for e.g. PLAIN representations.
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_fs_fs/transaction.c
> >
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1669743&r1=1669742&r2=1669743&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Sat Mar 28
> 10:58:13 2015
> > @@ -2214,10 +2214,11 @@ get_shared_rep(representation_t **old_re
> > return SVN_NO_ERROR;
> >
> > /* We don't want 0-length PLAIN representations to replace
> non-0-length
> > - ones (see issue #4554). Also, this doubles as a simple guard
> against
> > - general rep-cache induced corruption. */
> > + ones (see issue #4554). Take into account that EXPANDED_SIZE may
> be
> > + 0 in which case we have to check the on-disk SIZE. Also, this
> doubles
> > + as a simple guard against general rep-cache induced corruption. */
> > if ( ((*old_rep)->expanded_size != rep->expanded_size)
> > - || ((*old_rep)->size != rep->size))
> > + || (rep->expanded_size && ((*old_rep)->size != rep->size)))
>
> If two reps have EXPANDED_SIZE == 0 and different SIZE, the condition
> will be FALSE. Shouldn't it be TRUE instead?
>
Yikes! Forgot the "!" in front of rep->expanded_size
after updating the commentary. Fixed in r1669945.
When comparing SIZE, shouldn't it also compare the rep kind (PLAIN or
> DELTA) at the same time? Otherwise, two reps that have the same
> EXPANDED_SIZE, different SIZE, and different kinds wouldn't be shared,
> even though they could be.
>
The condition as it stands now on /trunk should
be minimal and correct.
Thanks for the review!
-- Stefan^2.
Re: svn commit: r1669743 - /subversion/trunk/subversion/libsvn_fs_fs/transaction.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Mar 29, 2015 at 9:44 AM, Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> stefan2@apache.org wrote on Sat, Mar 28, 2015 at 10:58:13 -0000:
> > Author: stefan2
> > Date: Sat Mar 28 10:58:13 2015
> > New Revision: 1669743
> >
> > URL: http://svn.apache.org/r1669743
> > Log:
> > Follow-up to r1654934:
> > Relax the size check in the FSFS representation sharing code to make it
> as
> > effective as in earlier releases.
> >
> > The size check has been to strict and would not allow two matching reps
> to
> > have a different base representation, e.g. mod on /trunk and add-without-
> > history on a branch. Mainly depending on the merge policy (catch-up vs.
> > cherry-pick), this resulted in detecting and eliding fewer instances of
> > redundant representations. Hence, larger repositories.
> >
> > * subversion/libsvn_fs_fs/transaction.c
> > (get_shared_rep): Care about the on-disk size only if the fulltext /
> > expanded size is not known - which would be the case
> > for e.g. PLAIN representations.
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_fs_fs/transaction.c
> >
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1669743&r1=1669742&r2=1669743&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Sat Mar 28
> 10:58:13 2015
> > @@ -2214,10 +2214,11 @@ get_shared_rep(representation_t **old_re
> > return SVN_NO_ERROR;
> >
> > /* We don't want 0-length PLAIN representations to replace
> non-0-length
> > - ones (see issue #4554). Also, this doubles as a simple guard
> against
> > - general rep-cache induced corruption. */
> > + ones (see issue #4554). Take into account that EXPANDED_SIZE may
> be
> > + 0 in which case we have to check the on-disk SIZE. Also, this
> doubles
> > + as a simple guard against general rep-cache induced corruption. */
> > if ( ((*old_rep)->expanded_size != rep->expanded_size)
> > - || ((*old_rep)->size != rep->size))
> > + || (rep->expanded_size && ((*old_rep)->size != rep->size)))
>
> If two reps have EXPANDED_SIZE == 0 and different SIZE, the condition
> will be FALSE. Shouldn't it be TRUE instead?
>
Yikes! Forgot the "!" in front of rep->expanded_size
after updating the commentary. Fixed in r1669945.
When comparing SIZE, shouldn't it also compare the rep kind (PLAIN or
> DELTA) at the same time? Otherwise, two reps that have the same
> EXPANDED_SIZE, different SIZE, and different kinds wouldn't be shared,
> even though they could be.
>
The condition as it stands now on /trunk should
be minimal and correct.
Thanks for the review!
-- Stefan^2.