Re: svn commit: r1730389 - /subversion/trunk/subversion/libsvn_repos/log.c

[Stefan Fuhrmann]

On 15.02.2016 09:39, Ivan Zhakov wrote:

On 14 February 2016 at 22:25,   wrote:

Author: stefan2
Date: Sun Feb 14 19:25:12 2016
New Revision: 1730389

URL: http://svn.apache.org/viewvc?rev=1730389=rev
Log:
Begin work on the svn_repos_get_logs4 to svn_repos_get_logs5 migration.

As a first step, we introduce what will become svn_repos_get_logs5 as
a static function and implement svn_repos_get_logs4 as a wrapper.


The svn_repos__get_logs5() doesn't have docstring, while as far I
understand it has very tricky semantic of delivering data using two
callbacks. Without documented svn_repos__get_logs5() API it's almost
impossible to get idea of all following commits in log.c


The API definition / migration is complete now and the next
step is publish it.  It'll then get a proper docstring.

-- Stefan^2.

RE: svn commit: r1730617 -/subversion/trunk/subversion/libsvn_repos/log.c

[Bert Huijben]

Are you sure the pool arguments are in the right order here?

The usual order is result_pool, scratch_pool while most of the calls here 
appear to use the opposite order.

Bert

Sent from Mail for Windows 10

From: stef...@apache.org
Sent: maandag 15 februari 2016 22:47
To: comm...@subversion.apache.org
Subject: svn commit: r1730617 -/subversion/trunk/subversion/libsvn_repos/log.c

Author: stefan2
Date: Mon Feb 15 21:47:00 2016
New Revision: 1730617

URL: http://svn.apache.org/viewvc?rev=1730617=rev
Log:
Continue work on the svn_repos_get_logs4 to svn_repos_get_logs5 migration:
Switch the last svn_fs_paths_changed2 call to svn_fs_paths_changed3.

* subversion/libsvn_repos/log.c
  (fs_mergeinfo_changed): No longer fetch the whole changes list.  However,
  we need to iterate twice for best total performance
  and we need to minimize FS iterator lifetimes.

Modified:
subversion/trunk/subversion/libsvn_repos/log.c

Modified: subversion/trunk/subversion/libsvn_repos/log.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/log.c?rev=1730617=1730616=1730617=diff
==
--- subversion/trunk/subversion/libsvn_repos/log.c (original)
+++ subversion/trunk/subversion/libsvn_repos/log.c Mon Feb 15 21:47:00 2016
@@ -631,11 +631,11 @@ fs_mergeinfo_changed(svn_mergeinfo_catal
  apr_pool_t *scratch_pool)
 {
   svn_fs_root_t *root;
-  apr_pool_t *iterpool;
-  apr_hash_index_t *hi;
+  apr_pool_t *iterpool, *iterator_pool;
+  svn_fs_path_change_iterator_t *iterator;
+  svn_fs_path_change3_t *change;
   svn_boolean_t any_mergeinfo = FALSE;
   svn_boolean_t any_copy = FALSE;
-  apr_hash_t *changes;
 
   /* Initialize return variables. */
   *deleted_mergeinfo_catalog = svn_hash__make(result_pool);
@@ -645,55 +645,69 @@ fs_mergeinfo_changed(svn_mergeinfo_catal
   if (rev == 0)
 return SVN_NO_ERROR;
 
+  /* FS iterators are potentially heavy objects.
+   * Hold them in a separate pool to clean them up asap. */
+  iterator_pool = svn_pool_create(scratch_pool);
+
   /* We're going to use the changed-paths information for REV to
  narrow down our search. */
   SVN_ERR(svn_fs_revision_root(, fs, rev, scratch_pool));
-  SVN_ERR(svn_fs_paths_changed2(, root, scratch_pool));
+  SVN_ERR(svn_fs_paths_changed3(, root, iterator_pool,
+scratch_pool));
+  SVN_ERR(svn_fs_path_change_get(, iterator));
 
   /* Look for copies and (potential) mergeinfo changes.
- We will use both flags to take shortcuts further down the road. */
-  for (hi = apr_hash_first(scratch_pool, changes);
-   hi;
-   hi = apr_hash_next(hi))
-{
-  svn_fs_path_change2_t *change = apr_hash_this_val(hi);
+ We will use both flags to take shortcuts further down the road.
 
+ The critical information here is whether there are any copies
+ because that greatly influences the costs for log processing.
+ So, it is faster to iterate over the changes twice - in the worst
+ case b/c most times there is no m/i at all and we exit out early
+ without any overhead. 
+   */
+  while (change && (!any_mergeinfo || !any_copy))
+{
   /* If there was a prop change and we are not positive that _no_
  mergeinfo change happened, we must assume that it might have. */
   if (change->mergeinfo_mod != svn_tristate_false && change->prop_mod)
 any_mergeinfo = TRUE;
 
-  switch (change->change_kind)
-{
-case svn_fs_path_change_add:
-case svn_fs_path_change_replace:
-  any_copy = TRUE;
-  break;
+  if (   (change->change_kind == svn_fs_path_change_add)
+  || (change->change_kind == svn_fs_path_change_replace))
+any_copy = TRUE;
 
-default:
-  break;
-}
+  SVN_ERR(svn_fs_path_change_get(, iterator));
 }
 
   /* No potential mergeinfo changes?  We're done. */
   if (! any_mergeinfo)
-return SVN_NO_ERROR;
+{
+  svn_pool_destroy(iterator_pool);
+  return SVN_NO_ERROR;
+}
+
+  /* There is or may be some m/i change. Look closely now. */
+  svn_pool_clear(iterator_pool);
+  SVN_ERR(svn_fs_paths_changed3(, root, iterator_pool,
+scratch_pool));
 
   /* Loop over changes, looking for anything that might carry an
  svn:mergeinfo change and is one of our paths of interest, or a
  child or [grand]parent directory thereof. */
   iterpool = svn_pool_create(scratch_pool);
-  for (hi = apr_hash_first(scratch_pool, changes);
-   hi;
-   hi = apr_hash_next(hi))
+  while (TRUE)
 {
   const char *changed_path;
-  svn_fs_path_change2_t *change = apr_hash_this_val(hi);
   const char *base_path = NULL;
   svn_revnum_t base_rev = SVN_INVALID_REVNUM;
   svn_fs_root_t *base_root = NULL;
   svn_string_t *prev_mergeinfo_value = NULL, *mergeinfo_value;
 
+  /* Next 

RE: svn commit: r1730546 - in /subversion/trunk/subversion:include/private/svn_wc_private.h libsvn_client/resolved.clibsvn_wc/conflicts.c

[Bert Huijben]

There shouldn’t be many cases where the wait is necessary, if any… Resolving a 
conflict never involves changing BASE and typically we already waited before 
invoking the resolver. So this could be as simple as just removing the waits. 
(I can’t really think of a case in tree/property conflicts where this would be 
needed. Perhaps some hard tree conflict resolve step would be an exception)

I was surprised to see that we have waits… and the functions that have the wait 
now are probably too low level to have the wait. An invocation of an svn_client 
api should have just 1 sleep, even on recursive operations.

Bert

Sent from Mail for Windows 10

From: Stefan Sperling
Sent: maandag 15 februari 2016 18:17
To: Bert Huijben
Cc: dev@subversion.apache.org
Subject: Re: svn commit: r1730546 - in 
/subversion/trunk/subversion:include/private/svn_wc_private.h 
libsvn_client/resolved.clibsvn_wc/conflicts.c

On Mon, Feb 15, 2016 at 05:30:55PM +0100, Bert Huijben wrote:
> Why would you need to wait for timestamps here?
> 
> Can this change the working copy file?
> 
> If it can the wc function should probably return a boolean to notify the case 
> where it does, and only when it does we should wait for timestamps.
> 

I agree that this should be fixed.

I'd like to spend some more time exploring the new APIs before addressing
performance issues like this. Hunting down all the code paths where we may
or may not modify a working copy file probably takes some time. I'm willing
to invest that time eventually, but not right now.

Please feel free to fix this yourself.
If you don't have time to deal with this problem right away, could you
please file an issue so we don't forget? Thanks!

Re: Merging parallel-put to /trunk

[Thomas Åkesson]

> I think that local commits are usually fast enough.  But committing over
> a high-latency network, e.g., with a transatlantic RTT of 150ms, can be
> painfully slow — see below.


Poor network connections are of course a very important concern, likely even 
more important than some additional seconds wait when committing large files on 
a perfect connection.

That said, I really don’t believe in the single-POST approach. Consider how 
that would work on a crappy connection when traveling on a train. 
TCP-connections are frequently broken, or worse, stalling. 

Going to parallel PUTs will likely address both issues:
 - Great connection: Multi core performance, very much in line with CPU 
development in recent years. Also competes better with other network traffic as 
more and more applications open many TCP-connection.
 - Poor connection: High latency, whatever, the waiting time gives more space 
for the other uploads (assuming that all parallel PUTs are not entirely in 
sync). 

Regards,
Thomas Å.

Re: svn commit: r1730546 - in /subversion/trunk/subversion: include/private/svn_wc_private.h libsvn_client/resolved.c libsvn_wc/conflicts.c

[Stefan Sperling]

On Mon, Feb 15, 2016 at 05:30:55PM +0100, Bert Huijben wrote:
> Why would you need to wait for timestamps here?
> 
> Can this change the working copy file?
> 
> If it can the wc function should probably return a boolean to notify the case 
> where it does, and only when it does we should wait for timestamps.
> 

I agree that this should be fixed.

I'd like to spend some more time exploring the new APIs before addressing
performance issues like this. Hunting down all the code paths where we may
or may not modify a working copy file probably takes some time. I'm willing
to invest that time eventually, but not right now.

Please feel free to fix this yourself.
If you don't have time to deal with this problem right away, could you
please file an issue so we don't forget? Thanks!

RE: svn commit: r1730546 - in /subversion/trunk/subversion: include/private/svn_wc_private.h libsvn_client/resolved.c libsvn_wc/conflicts.c

[Bert Huijben]

See inline.

> -Original Message-
> From: s...@apache.org [mailto:s...@apache.org]
> Sent: maandag 15 februari 2016 16:04
> To: comm...@subversion.apache.org
> Subject: svn commit: r1730546 - in /subversion/trunk/subversion:
> include/private/svn_wc_private.h libsvn_client/resolved.c
> libsvn_wc/conflicts.c
> 
> Author: stsp
> Date: Mon Feb 15 15:04:10 2016
> New Revision: 1730546
> 
> URL: http://svn.apache.org/viewvc?rev=1730546=rev
> Log:
> Provide new private libsvn_wc APIs for resolving text and property conflicts.
> Use these new APIs from libsvn_client's new conflict resolver instead of
> calling the generic svn_wc__resolve_conflicts() function.
> 
> This avoids going on a status walk for just one path at depth empty.
> The new functions added here provide sufficient functionality for the new
> conflict resolver: Marking a text/prop conflict resolved based on a choice
> made by the user, and sending a notification to the client.
> 
> For now, tree conflicts are still resolved with svn_wc__resolve_conflicts().
> The plan is to add several new libsvn_wc APIs for resolving tree conflicts.
> These APIs will not be driven by a simple conflict choice argument. Rather,
> each API will implement a very specific resolution strategy for a particular
> kind of tree conflict.
> Eventually, libsvn_client will stop using svn_wc__resolve_conflicts() for
> anything but backwards compatibility.
> 
> * subversion/include/private/svn_wc_private.h
>   (svn_wc__conflict_text_mark_resolved,
>svn_wc__conflict_prop_mark_resolved): Declare.
> 
> * subversion/libsvn_client/resolved.c
>   (resolve_text_conflict): Call svn_wc__conflict_text_mark_resolved().
>   (resolve_prop_conflict): Call svn_wc__conflict_prop_mark_resolved().
>   (resolve_tree_conflict): Inline the body of resolve_conflict() here.
>   (resolve_conflict): Remove.
> 
> * subversion/libsvn_wc/conflicts.c
>   (svn_wc__conflict_text_mark_resolved,
>svn_wc__conflict_prop_mark_resolved): Implement.
> 
> Modified:
> subversion/trunk/subversion/include/private/svn_wc_private.h
> subversion/trunk/subversion/libsvn_client/resolved.c
> subversion/trunk/subversion/libsvn_wc/conflicts.c
> 
> Modified: subversion/trunk/subversion/include/private/svn_wc_private.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private
> /svn_wc_private.h?rev=1730546=1730545=1730546=diff
> ==
> 
> --- subversion/trunk/subversion/include/private/svn_wc_private.h (original)
> +++ subversion/trunk/subversion/include/private/svn_wc_private.h Mon
> Feb 15 15:04:10 2016
> @@ -1748,6 +1748,39 @@ svn_wc__resolve_conflicts(svn_wc_context
>void *notify_baton,
>apr_pool_t *scratch_pool);
> 
> +/**
> + * Resolve the text conflict at LOCAL_ABSPATH as per CHOICE, and then
> + * mark the conflict resolved.
> + * The working copy must already be locked for resolving, e.g. by calling
> + * svn_wc__acquire_write_lock_for_resolve() first.
> + * @since New in 1.10.
> + */
> +svn_error_t *
> +svn_wc__conflict_text_mark_resolved(svn_wc_context_t *wc_ctx,
> +const char *local_abspath,
> +svn_wc_conflict_choice_t choice,
> +svn_cancel_func_t cancel_func,
> +void *cancel_baton,
> +svn_wc_notify_func2_t notify_func,
> +void *notify_baton,
> +apr_pool_t *scratch_pool);
> +
> +/**
> + * Resolve the conflicted property PROPNAME at LOCAL_ABSPATH as per
> CHOICE,
> + * and then mark the conflict resolved.
> + * The working copy must already be locked for resolving, e.g. by calling
> + * svn_wc__acquire_write_lock_for_resolve() first.
> + * @since New in 1.10.
> + */
> +svn_error_t *
> +svn_wc__conflict_prop_mark_resolved(svn_wc_context_t *wc_ctx,
> +const char *local_abspath,
> +const char *propname,
> +svn_wc_conflict_choice_t choice,
> +svn_wc_notify_func2_t notify_func,
> +void *notify_baton,
> +apr_pool_t *scratch_pool);
> +
>  /**
>   * Move @a src_abspath to @a dst_abspath, by scheduling @a dst_abspath
>   * for addition to the repository, remembering the history. Mark @a
> src_abspath
> 
> Modified: subversion/trunk/subversion/libsvn_client/resolved.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/re
> solved.c?rev=1730546=1730545=1730546=diff
> ==
> 
> --- subversion/trunk/subversion/libsvn_client/resolved.c (original)
> +++ 

Re: SVN::Client::log() first argument Re: svn commit: r1729519 - /subversion/trunk/tools/client-side/svn-graph.pl

[James McCoy]

On Mon, Feb 15, 2016 at 11:54:39AM +0100, Bert Huijben wrote:
> > -Original Message-
> > From: James McCoy [mailto:vega.ja...@gmail.com] On Behalf Of James
> > McCoy
> > Sent: zondag 14 februari 2016 19:20
> > To: Daniel Shahaf 
> > Cc: dev@subversion.apache.org
> > Subject: Re: SVN::Client::log() first argument Re: svn commit: r1729519 -
> > /subversion/trunk/tools/client-side/svn-graph.pl
> > 
> > On Sun, Feb 14, 2016 at 02:34:42PM +, Daniel Shahaf wrote:
> > > james...@apache.org wrote on Wed, Feb 10, 2016 at 03:22:36 -:
> > > ># Retrieve the requested history.
> > > > -  $ra->get_log([''], $startrev, $youngest, 0, 1, 0, 
> > > > \_revision);
> > > > +  $client->log($repos_url, $startrev, $youngest, 1, 0,
> > \_revision);
> > >
> > > Why does this work?  Both svn_client.h and SVN::Client(3) state the
> > > first argument is "targets", plural.
> > 
> > SVN::Client(3) describes targets as
> > 
> >$targets
> >This argument can either be a single $target (as defined
> >above) or a reference to an array of them.
> 
> Do you have any pointers to see how this is implemented?

subversion/bindings/swig/include/svn_containers.swg defines a typemap
for "const apr_array_header_t *" parameters:

%typemap(in) const apr_array_header_t *STRINGLIST {
$1 = svn_swig_pl_strings_to_array($input,
 _global_pool);
}
%typemap(in) const apr_array_header_t *STRINGLIST_MAY_BE_NULL {
$1 = SvOK($input) ? svn_swig_pl_strings_to_array(
$input, _global_pool) : NULL;
}

svn_swig_pl_strings_to_array (via svn_swig_pl_to_array), in
subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c, handles
ensuring an apr_array_header_t is passed in to Subversion's APIs.

static apr_array_header_t *svn_swig_pl_to_array(SV *source,
pl_element_converter_t cv,
void *ctx, apr_pool_t *pool)
{
...
} else if (SvOK(source)) {
targlen = 1;
temp = apr_array_make(pool, targlen, sizeof(const char *));
temp->nelts = targlen;
APR_ARRAY_IDX(temp, 0, const char *) = cv(source, ctx, pool);
} else {

Cheers,
-- 
James
GPG Key: 4096R/331BA3DB 2011-12-05 James McCoy 

RE: SVN::Client::log() first argument Re: svn commit: r1729519 - /subversion/trunk/tools/client-side/svn-graph.pl

[Bert Huijben]

> -Original Message-
> From: James McCoy [mailto:vega.ja...@gmail.com] On Behalf Of James
> McCoy
> Sent: zondag 14 februari 2016 19:20
> To: Daniel Shahaf 
> Cc: dev@subversion.apache.org
> Subject: Re: SVN::Client::log() first argument Re: svn commit: r1729519 -
> /subversion/trunk/tools/client-side/svn-graph.pl
> 
> On Sun, Feb 14, 2016 at 02:34:42PM +, Daniel Shahaf wrote:
> > james...@apache.org wrote on Wed, Feb 10, 2016 at 03:22:36 -:
> > ># Retrieve the requested history.
> > > -  $ra->get_log([''], $startrev, $youngest, 0, 1, 0, \_revision);
> > > +  $client->log($repos_url, $startrev, $youngest, 1, 0,
> \_revision);
> >
> > Why does this work?  Both svn_client.h and SVN::Client(3) state the
> > first argument is "targets", plural.
> 
> SVN::Client(3) describes targets as
> 
>$targets
>  This argument can either be a single $target (as defined
>  above) or a reference to an array of them.

Do you have any pointers to see how this is implemented?

I can find this documentation specific for 'targets' argument in one of our .i 
files, but no matching implementation.

I'm trying to find why, but the only things I can find would indicate that this 
is just based on how swig implements array mappings for perl.

Bert

Re: What's an svnserve groups-db file? Fwd: [C. Michael Pilato: Re: [svnbook] r5084 committed - trunk/en/book/ch06-server-configuration.xml]

[Evgeny Kotkov]

Daniel Shahaf  writes:

> From the default svnserve.conf:
>
> ### The groups-db option controls the location of the groups file.
> ### Unless you specify a path starting with a /, the file's location is
> ### relative to the directory containing this file.  The specified path
> ### may be a repository relative URL (^/) or an absolute file:// URL to a
> ### text file in a Subversion repository.
> # groups-db = groups
>
> Could whoever implemented that knob please document the syntax and
> semantics of a groups-db file?  Does the file use the ini format?  Does
> it have an ini section header?  What happens if both an authz-db file
> containing a [groups] section and a groups-db file are configured?
>
> This information doesn't belong on the mailing list; it belongs in the
> 1.8 release notes, the 1.8 svnbook, or the template svnserve.conf file's
> comments.

Will do.


Regards,
Evgeny Kotkov

Re: svn commit: r1730389 - /subversion/trunk/subversion/libsvn_repos/log.c

[Ivan Zhakov]

On 14 February 2016 at 22:25,   wrote:
> Author: stefan2
> Date: Sun Feb 14 19:25:12 2016
> New Revision: 1730389
>
> URL: http://svn.apache.org/viewvc?rev=1730389=rev
> Log:
> Begin work on the svn_repos_get_logs4 to svn_repos_get_logs5 migration.
>
> As a first step, we introduce what will become svn_repos_get_logs5 as
> a static function and implement svn_repos_get_logs4 as a wrapper.

The svn_repos__get_logs5() doesn't have docstring, while as far I
understand it has very tricky semantic of delivering data using two
callbacks. Without documented svn_repos__get_logs5() API it's almost
impossible to get idea of all following commits in log.c


-- 
Ivan Zhakov