On Sun, Mar 29, 2015 at 9:44 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name>
wrote:
> stefan2_at_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.
Received on 2015-03-29 20:18:54 CEST