subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bert Huijben <b...@qqmail.nl>
Subject RE: svn commit: r1730617 -/subversion/trunk/subversion/libsvn_repos/log.c
Date Mon, 15 Feb 2016 22:45:24 GMT
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: stefan2@apache.org
Sent: maandag 15 februari 2016 22:47
To: commits@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&view=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&r1=1730616&r2=1730617&view=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(&root, fs, rev, scratch_pool));
-  SVN_ERR(svn_fs_paths_changed2(&changes, root, scratch_pool));
+  SVN_ERR(svn_fs_paths_changed3(&iterator, root, iterator_pool,
+                                scratch_pool));
+  SVN_ERR(svn_fs_path_change_get(&change, 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(&change, 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(&iterator, 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 change. */
+      SVN_ERR(svn_fs_path_change_get(&change, iterator));
+      if (!change)
+        break;
+
       /* Cheap pre-checks that don't require memory allocation etc. */
 
       /* No mergeinfo change? -> nothing to do here. */
@@ -705,7 +719,7 @@ fs_mergeinfo_changed(svn_mergeinfo_catal
         continue;
 
       /* Begin actual processing */
-      changed_path = apr_hash_this_key(hi);
+      changed_path = change->path.data;
       svn_pool_clear(iterpool);
 
       switch (change->change_kind)
@@ -863,6 +877,8 @@ fs_mergeinfo_changed(svn_mergeinfo_catal
     }
 
   svn_pool_destroy(iterpool);
+  svn_pool_destroy(iterator_pool);
+
   return SVN_NO_ERROR;
 }
 




Mime
View raw message