[PATCH] Fix an inefficient way to fill an array of inherited properties

[Denis Kovalchuk <de...@visualsvn.com>]

Hello.

I think that currently the code that obtains inherited properties for servers
that don't support the capability uses an inefficient way of array filling.
At each iteration, all previously added properties are moved one position to
the right to maintain depth-first order. I think it's possible to provide a
similar behavior walking the array of iprop requests from the end and get rid
of extra copying of array elements.

I've attached a patch.

Regards,
Denis Kovalchuk

Re: [PATCH] Fix an inefficient way to fill an array of inherited properties

[Daniel Shahaf <d....@daniel.shahaf.name>]

Nathan Hartman wrote on Mon, 25 May 2020 11:18 -0400:
> Bringing this portion from [1] back to this thread:
> 
> On Sun, May 24, 2020 at 8:38 PM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Nathan Hartman wrote on Sun, 24 May 2020 18:28 -0400:  
> > > Asking because I ran the latter on latest trunk with this change
> > > applied. (All tests passed.) Also from reading the code, including
> > > spelunking into the APR calls etc, I see no functional difference
> > > (except efficiency as stated).  
> >
> > "except efficiency"?  
> 
> :-)
> 
> I meant: I see no functional difference, except that this
> implementation should construct the array more efficiently than the
> previous one.

Thanks for clarifying.  Based on the (snipped) context, I thought you
had been referring to some difference between davautocheck stsp's
Makefile's serf-check, especially given that the issues I was reporting
could be reproduced in unpatched trunk@HEAD [which, at the time, didn't
contain the patch in the OP] as well.

Re: [PATCH] Fix an inefficient way to fill an array of inherited properties

[Nathan Hartman <ha...@gmail.com>]

Bringing this portion from [1] back to this thread:

On Sun, May 24, 2020 at 8:38 PM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Nathan Hartman wrote on Sun, 24 May 2020 18:28 -0400:
> > Asking because I ran the latter on latest trunk with this change
> > applied. (All tests passed.) Also from reading the code, including
> > spelunking into the APR calls etc, I see no functional difference
> > (except efficiency as stated).
>
> "except efficiency"?

:-)

I meant: I see no functional difference, except that this
implementation should construct the array more efficiently than the
previous one.

Nathan

[1] "Sporadic davautocheck failure in trunk? (was: Re: [PATCH] Fix an
inefficient way to fill an array of inherited properties)" 24 May 2020
https://lists.apache.org/thread.html/r7e332a8d760527d754d4a5d23094d7712605a4eb10e76a8e91f1f2c5%40%3Cdev.subversion.apache.org%3E

Sporadic davautocheck failure in trunk? (was: Re: [PATCH] Fix an inefficient way to fill an array of inherited properties)

[Daniel Shahaf <d....@daniel.shahaf.name>]

Nathan Hartman wrote on Sun, 24 May 2020 18:28 -0400:
> On Sun, May 24, 2020 at 6:00 PM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> >
> > Nathan Hartman wrote on Sun, 24 May 2020 12:10 -0400:  
> > > On Sat, May 23, 2020 at 8:38 PM Branko Čibej <br...@apache.org> wrote:  
> > > > For my part, I just didn't get around to it. I think it should be committed.  
> > >
> > > I committed it in r1878084.  
> >
> > Thanks, Nathan.  I'd have done this myself, but my build environment
> > had bitrotted to the point that davautocheck didn't pass.  
> 
> Uh oh, that sounds bad.
> 

Well, the 1.14 pre-releases built fine for everyone else (probably
including several developers on OS's similar to mine), the bots were
green, and I had just freshly re-installed the apache2 packages, so I
figured it was _probably_ a bug in my environment.

But yeah, it's a probability.

I haven't kept fails.log, unfortunately.  I built on Debian stable.
I'd have built with my usual array of flags¹, run «make davautocheck»
with «APACHE_MPM=event THREADS=1 CLEANUP=1», and confirmed that the
failure happened without the patch as well (otherwise I'd have posted
then).  From memory, the failure happened approximately once per
davautocheck run (with the «TESTS» Makefile parameter unspecified,
i.e., once per full run), not always at the same place.  

> I haven't looked at davautocheck.sh before. Does it test anything that
> tools/dev/unix-build/Makefile.svn doesn't?

davautocheck.sh is just a script that sets up httpd and runs the test
suite with a --url= argument pointing to that httpd.

I don't know how «make davautocheck» compares to «make -f …/unix-build/Makefile.svn
serf-check».

davautocheck.sh has some development-oriented flags, documented in
comments at the top of the script.

> Asking because I ran the latter on latest trunk with this change
> applied. (All tests passed.) Also from reading the code, including
> spelunking into the APR calls etc, I see no functional difference
> (except efficiency as stated).

"except efficiency"?

Cheers,

Daniel

¹ «CONFIG_SHELL=/bin/dash /path/to/configure AR_FLAGS=cr RUBY=none
  --disable-nls -q -C --enable-maintainer-mode 'CFLAGS=-DSVN_DEPRECATED=
  -DSVN_FS_FS_DEFAULT_MAX_FILES_PER_DIR=3 -DPACK_AFTER_EVERY_COMMIT
  -DSVN_UNALIGNED_ACCESS_IS_OK=0 -DSUFFIX_LINES_TO_KEEP=0 '», builddir
  on a tmpfs.  With these flags, three svnadmin tests are _expected_ to
  fail; those failures are unrelated to the other failures discussed
  above.

Re: [PATCH] Fix an inefficient way to fill an array of inherited properties

[Nathan Hartman <ha...@gmail.com>]

On Sun, May 24, 2020 at 6:00 PM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>
> Nathan Hartman wrote on Sun, 24 May 2020 12:10 -0400:
> > On Sat, May 23, 2020 at 8:38 PM Branko Čibej <br...@apache.org> wrote:
> > > For my part, I just didn't get around to it. I think it should be committed.
> >
> > I committed it in r1878084.
>
> Thanks, Nathan.  I'd have done this myself, but my build environment
> had bitrotted to the point that davautocheck didn't pass.

Uh oh, that sounds bad.

I haven't looked at davautocheck.sh before. Does it test anything that
tools/dev/unix-build/Makefile.svn doesn't? Asking because I ran the
latter on latest trunk with this change applied. (All tests passed.)
Also from reading the code, including spelunking into the APR calls
etc, I see no functional difference (except efficiency as stated).

Nathan

Re: [PATCH] Fix an inefficient way to fill an array of inherited properties

[Daniel Shahaf <d....@daniel.shahaf.name>]

Nathan Hartman wrote on Sun, 24 May 2020 12:10 -0400:
> On Sat, May 23, 2020 at 8:38 PM Branko Čibej <br...@apache.org> wrote:
> > For my part, I just didn't get around to it. I think it should be committed.  
> 
> I committed it in r1878084.

Thanks, Nathan.  I'd have done this myself, but my build environment
had bitrotted to the point that davautocheck didn't pass.

Re: [PATCH] Fix an inefficient way to fill an array of inherited properties

[Nathan Hartman <ha...@gmail.com>]

On Sat, May 23, 2020 at 8:38 PM Branko Čibej <br...@apache.org> wrote:
> For my part, I just didn't get around to it. I think it should be committed.

I committed it in r1878084.

Nathan

Re: [PATCH] Fix an inefficient way to fill an array of inherited properties

[Branko Čibej <br...@apache.org>]

On 24.05.2020 00:17, Daniel Shahaf wrote:
> Branko Čibej wrote on Fri, 15 May 2020 17:07 +0200:
>> On 15.05.2020 16:54, Denis Kovalchuk wrote:
>>> Hello.
>>>
>>> I think that currently the code that obtains inherited properties for servers
>>> that don't support the capability uses an inefficient way of array filling.
>>> At each iteration, all previously added properties are moved one position to
>>> the right to maintain depth-first order. I think it's possible to provide a
>>> similar behavior walking the array of iprop requests from the end and get rid
>>> of extra copying of array elements.
>>>
>>> I've attached a patch.  
>>
>> Looks like an obvious fix. +1
> Any reason this hasn't been committed?

For my part, I just didn't get around to it. I think it should be committed.

-- Brane

Re: [PATCH] Fix an inefficient way to fill an array of inherited properties

[Daniel Shahaf <d....@daniel.shahaf.name>]

Branko Čibej wrote on Fri, 15 May 2020 17:07 +0200:
> On 15.05.2020 16:54, Denis Kovalchuk wrote:
> > Hello.
> >
> > I think that currently the code that obtains inherited properties for servers
> > that don't support the capability uses an inefficient way of array filling.
> > At each iteration, all previously added properties are moved one position to
> > the right to maintain depth-first order. I think it's possible to provide a
> > similar behavior walking the array of iprop requests from the end and get rid
> > of extra copying of array elements.
> >
> > I've attached a patch.  
> 
> 
> Looks like an obvious fix. +1

Any reason this hasn't been committed?

Re: [PATCH] Fix an inefficient way to fill an array of inherited properties

[Branko Čibej <br...@apache.org>]

On 15.05.2020 16:54, Denis Kovalchuk wrote:
> Hello.
>
> I think that currently the code that obtains inherited properties for servers
> that don't support the capability uses an inefficient way of array filling.
> At each iteration, all previously added properties are moved one position to
> the right to maintain depth-first order. I think it's possible to provide a
> similar behavior walking the array of iprop requests from the end and get rid
> of extra copying of array elements.
>
> I've attached a patch.


Looks like an obvious fix. +1

-- Brane