subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
Subject Re: svn commit: r1694533 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Date Wed, 26 Aug 2015 13:06:13 GMT
On Wed, Aug 26, 2015 at 1:31 PM, Ivan Zhakov <ivan@visualsvn.com> wrote:

> On 26 August 2015 at 11:18, Branko ─îibej <brane@wandisco.com> wrote:
> > On 25.08.2015 23:08, Stefan Fuhrmann wrote:
> >>> All right, so I figured that the difference is that apr_array_make does
> >>> two allocations compared to one in this code. Still: relying on
> >>> knowledge of APR implementation details is a really bad idea.
> >>
> >> The structure definition of apr_array_header_t is part of
> >> the public API, i.e. will never change.
> >
> > The semantics might, even if the shape of the structure itself doesn't.
> >
> >>> As it
> >>> stands, APR will correctly resize this array if necessary; but there's
> >>> no guarantee that, sometime in the future, resizing such arrays would
> >>> fail horribly.
> >>>
> >> Even if that was true, the resize is done outside APR as
> >> well - a few lines further down.
> >
> > Indeed it is ... and that code is essentially a copy-paste from
> > apr_tables.c.
> >
> >
> > I still think this kind of performance hack belongs in APR. Users that
> > don't have a new-enough APR won't get the performance boost, but on the
> > other hand, the kind of bug that started this discussion will stay out
> > of our code.
> >
> > I think we've had our fair share of alignment bugs with all the
> > hand-crafted allocations etc.; we may as well stop now.
> >
> +1.
>

Given the clear feedback, I reverted the change on /trunk and
proposed the good ol' code for backport.

For 1.10, I'll replace svn_ra_svn_item_t within the ra_svn code
with one that uses a plain C array instead the APR one - we simply
don't need their resizing feature etc.

-- Stefan^2.

Mime
View raw message