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

Make FIFO accounting_method linear time #115

Merged
merged 6 commits into from
Jun 1, 2024
Merged

Conversation

qwhelan
Copy link
Contributor

@qwhelan qwhelan commented May 14, 2024

The FIFO accounting method is unfortunately quadratic time, even in trivial cases:

One In, One Out
          FIFO      LIFO      HIFO
1     0.000234  0.000234  0.000231
10    0.001319  0.001319  0.001431
100   0.020807  0.013064  0.022888
1000  1.073289  0.129614  1.145311

Buys, then Sells
          FIFO      LIFO      HIFO
1     0.000225  0.000236  0.000239
10    0.001399  0.001412  0.001534
100   0.022002  0.023371  0.031054
1000  1.119583  1.103518  1.977542

The "One In, One Out" alternates n pairs of BUY, SELL such that there is only ever k=1 active lots. This represents the most trivial case for any accounting method. A potential worst case is represented by "Buys, then Sells" which creates n BUYS followed by n SELLS. This creates a worst case scenario of k=n.

In the first case, the LIFO method is roughly linear; in the second case, it is clearly quadratic. The quadratic behavior of LIFO and HIFO will be addressed in a separate PR by using heapq.

It is relatively straightforward to make FIFO linear time in both of these cases by simply advancing an index of the next, un-exhausted lot on AcquiredLotCandidates and persisting this object between invocations of get_acquired_lot_for_taxable_event(). In the event of different accounting methods being used for different tax years, a one-time scan is performed for each switch of accounting method to find the appropriate index.

With this PR, FIFO performance improves to linear time in both cases:

One In, One Out
          FIFO      LIFO      HIFO
1     0.000245  0.000248  0.000242
10    0.001359  0.001396  0.001423
100   0.012248  0.012903  0.022407
1000  0.127431  0.133112  1.161365

Buys, then Sells
          FIFO      LIFO      HIFO
1     0.000233  0.000258  0.000243
10    0.001345  0.001436  0.001533
100   0.012368  0.023112  0.031476
1000  0.129783  1.185451  2.029749

Copy link
Owner

@eprbell eprbell left a comment

Choose a reason for hiding this comment

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

Thanks for working on accounting method performance: this was a known issue and it's great to have proposals in this area.

Accounting method management is a critical part of the engine, so I'm still going through the PR carefully to understand all the implications. This is just initial feedback (nits, mostly): as soon as I finish studying your code, I'd like to brainstorm this with you to make sure we're getting everything right before merging. So give me a few days and I'll post a follow up here.

src/rp2/abstract_accounting_method.py Outdated Show resolved Hide resolved
src/rp2/accounting_engine.py Outdated Show resolved Hide resolved
src/rp2/abstract_accounting_method.py Outdated Show resolved Hide resolved
@qwhelan
Copy link
Contributor Author

qwhelan commented May 18, 2024

@eprbell Of course, no rush. In the event it's useful, I've uploaded #116 so you can check out the LIFO/HIFO changes as well. The latter can wait until this is merged if it over-complicates things.

Copy link
Owner

@eprbell eprbell left a comment

Choose a reason for hiding this comment

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

This is a great improvement! I finally got some time to sit and study the change and I couldn't find any major issues with this approach. I have a few nits here and there, but this is almost ready to merge, I think.

One request: if possible, could you just git push future changes without force? Force makes reviewing a bit harder because I have to start from scratch every time (which is not a major problem with a small change like this, but still). Thanks!

src/rp2/accounting_engine.py Show resolved Hide resolved
src/rp2/accounting_engine.py Outdated Show resolved Hide resolved
src/rp2/abstract_accounting_method.py Outdated Show resolved Hide resolved
src/rp2/plugin/accounting_method/fifo.py Outdated Show resolved Hide resolved
@qwhelan
Copy link
Contributor Author

qwhelan commented May 30, 2024

@eprbell Sure, didn't realize GitHub made it hard to review. The force-pushing was mostly cleaning up early commits with issues I realized later, so I'll try to avoid going forward.

@qwhelan
Copy link
Contributor Author

qwhelan commented May 31, 2024

@eprbell Should be ready for final review/merge now

Copy link
Owner

@eprbell eprbell left a comment

Choose a reason for hiding this comment

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

LGTM. This is a really good improvement, thanks for doing it!

@eprbell eprbell merged commit 53ff552 into eprbell:main Jun 1, 2024
19 checks passed
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.

None yet

2 participants