Skip to content

Commit

Permalink
Revert "[account.cpp] gnc_account_remove_split searches from the end"
Browse files Browse the repository at this point in the history 
This reverts commit 5aff4fb. Was not
tested properly...
  • Loading branch information
@christopherlam
christopherlam committed May 21, 2024
1 parent 5aff4fb commit 038405b
Showing 1 changed file with 2 additions and 5 deletions.
7 changes: 2 additions & 5 deletions libgnucash/engine/Account.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2002,11 +2002,8 @@ gnc_account_remove_split (Account *acc, Split *s)

if (!g_hash_table_remove (priv->splits_hash, s))
return false;

// search splits in reverse, because removing the latest split is
// more common (e.g. from UI or during book shutdown)
auto rit = std::remove(priv->splits.rbegin(), priv->splits.rend(), s);
priv->splits.erase(rit.base(), priv->splits.end());
auto it = std::remove (priv->splits.begin(), priv->splits.end(), s);
priv->splits.erase (it, priv->splits.end());
//FIXME: find better event type
qof_event_gen(&acc->inst, QOF_EVENT_MODIFY, nullptr);
// And send the account-based event, too
Expand Down

7 comments on commit 038405b

@christopherlam
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works but looks like a bad hack!

diff --git a/libgnucash/engine/Account.cpp b/libgnucash/engine/Account.cpp
index e5761c027c..20ff479684 100644
--- a/libgnucash/engine/Account.cpp
+++ b/libgnucash/engine/Account.cpp
@@ -2002,8 +2002,13 @@ gnc_account_remove_split (Account *acc, Split *s)
 
     if (!g_hash_table_remove (priv->splits_hash, s))
         return false;
-    auto it = std::remove (priv->splits.begin(), priv->splits.end(), s);
-    priv->splits.erase (it, priv->splits.end());
+
+    if (priv->splits.back() == s)
+        priv->splits.pop_back();
+    else
+        priv->splits.erase (std::remove (priv->splits.begin(), priv->splits.end(), s),
+                            priv->splits.end());
+
     //FIXME: find better event type
     qof_event_gen(&acc->inst, QOF_EVENT_MODIFY, nullptr);
     // And send the account-based event, too

@christopherlam
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok it may be a hack but it's effective in speeding up the majority calls to remove_split. Any objections to specialising the last vector element prune?

@jralls
Copy link
Member

@jralls jralls commented on 038405b May 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was wrong with 5aff4fb?

Why not just leave the account splits vector alone when deleting transactions during book shutdown and let the account clear the vector when it's destroyed?

@christopherlam
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was wrong with 5aff4fb?

It causes failures https://github.com/Gnucash/gnucash/actions/runs/9174735832/job/25226214027

Why not just leave the account splits vector alone when deleting transactions during book shutdown and let the account clear the vector when it's destroyed?

Because qof_instance_get_destroying(acc) isn't set yet while transactions clear the splits.

@jralls
Copy link
Member

@jralls jralls commented on 038405b May 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong thing to check. qof_book_shutting_down should be already set. If it's not that's a separate problem.

@christopherlam
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to bypass the split operations if qof_book_shutting_down but the benchmark show e80249c improves from 0.6s to 0.32s and using qof_book_shutting_down to bypass pop_back() improves to 0.31s only.

@jralls
Copy link
Member

@jralls jralls commented on 038405b May 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, they're both doing the same thing, avoiding creation of the iterators. You complain that the pop_back way looks like a bad hack, so I offered a different way.

But either way it's hard to get excited over 1/3 second speedup when shutting down the program.

Please sign in to comment.