Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[engine.i] even faster gnc_get_match_commodity_splits #1926

Conversation

christopherlam
Copy link
Contributor

gnc_get_match_commodity_splits needs to scan the account splitlists to find suitable splits upto end_date. We can stop scanning each account at end_date because the splitlists are sorted by date. This saves significant time because majority of splits can by bypassed in the early report dates.

My primary book net worth linechart benchmarks -- fastest speeds from 1.05s to 0.54s, now practically unnoticeable

@christopherlam christopherlam force-pushed the even-faster-gnc_get_match_commodity_splits branch 2 times, most recently from daa7430 to faab3c6 Compare April 23, 2024 14:42
bindings/engine.i Outdated Show resolved Hide resolved
@christopherlam christopherlam force-pushed the even-faster-gnc_get_match_commodity_splits branch 2 times, most recently from 1443c36 to 9fb9afa Compare April 24, 2024 14:20
bindings/engine.i Outdated Show resolved Hide resolved
@christopherlam
Copy link
Contributor Author

no idea why it won't compile on github... all passes well on my pop_os end.

@jralls
Copy link
Member

jralls commented Apr 24, 2024

no idea why it won't compile on github... all passes well on my pop_os end.

It's just macOS (so far anyway). That's the problem I posted about on IRC.

@christopherlam christopherlam force-pushed the even-faster-gnc_get_match_commodity_splits branch from 641e7aa to 4c379f7 Compare April 25, 2024 01:25
@christopherlam christopherlam marked this pull request as ready for review April 25, 2024 04:09
@christopherlam christopherlam force-pushed the even-faster-gnc_get_match_commodity_splits branch 3 times, most recently from f1bb89b to e46d022 Compare April 28, 2024 15:13
@jralls
Copy link
Member

jralls commented Apr 28, 2024

What's going wrong with std::lower_bound? It's not really the distro, of course, it's some version of libstdc++ that has a problem. Does it work the way you expect in some other version?

@christopherlam
Copy link
Contributor Author

What's going wrong with std::lower_bound? It's not really the distro, of course, it's some version of libstdc++ that has a problem. Does it work the way you expect in some other version?

Beats me... Try reverting the fix-up commit and push: The Ubuntu 22.04 ci shows lots of test errors proving that the lower_bound approach is giving incorrect results!

@christopherlam
Copy link
Contributor Author

@jralls
Copy link
Member

jralls commented Apr 28, 2024

he Ubuntu 22.04 ci shows lots of test errors proving that the lower_bound approach is giving incorrect results!

You got the comparison backwards in is_after_date. std::lower_bound returns the first iterator where the passed-in function returns false so the comparison needs to be txn_date <= end_date. Alternatively change to std::upper_bound and reverse the parameters in is_after_date. I think the latter better expresses the intent because you're evaluating [begin(), is_after_date_iter); lower_bound would make sense for [is_after_date_iter, end()).

@christopherlam christopherlam force-pushed the even-faster-gnc_get_match_commodity_splits branch from 5c620a0 to ad9ca36 Compare April 29, 2024 03:21
@christopherlam
Copy link
Contributor Author

he Ubuntu 22.04 ci shows lots of test errors proving that the lower_bound approach is giving incorrect results!

You got the comparison backwards in is_after_date. std::lower_bound returns the first iterator where the passed-in function returns false so the comparison needs to be txn_date <= end_date. Alternatively change to std::upper_bound and reverse the parameters in is_after_date. I think the latter better expresses the intent because you're evaluating [begin(), is_after_date_iter); lower_bound would make sense for [is_after_date_iter, end()).

Ok you're right :-)

@christopherlam
Copy link
Contributor Author

The next question is why did arch ci pass the tests then...

@christopherlam christopherlam force-pushed the even-faster-gnc_get_match_commodity_splits branch from ad9ca36 to 78ad530 Compare April 29, 2024 13:37
@jralls jralls mentioned this pull request Apr 29, 2024
@christopherlam christopherlam force-pushed the even-faster-gnc_get_match_commodity_splits branch from 78ad530 to e53dfa6 Compare April 30, 2024 00:42
- uses binary search to find first split after date
- for_each from earliest split to (but excluding) the above first split
gnc_get_match_commodity_splits needs to scan the account splitlists to
find suitable splits upto end_date. Because splitlist is date-sorted,
use gnc_account_foreach_split_until_date to avoid scanning later
splits.
@christopherlam christopherlam force-pushed the even-faster-gnc_get_match_commodity_splits branch from e53dfa6 to 4c29f69 Compare May 2, 2024 14:21
@code-gnucash-org code-gnucash-org merged commit 4c29f69 into Gnucash:stable May 2, 2024
2 of 4 checks passed
@christopherlam christopherlam deleted the even-faster-gnc_get_match_commodity_splits branch May 2, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants