-
Notifications
You must be signed in to change notification settings - Fork 844
[Transaction.hpp] restore trans->is_closing_cached struct member #2070
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
base: stable
Are you sure you want to change the base?
Conversation
c1181a0
to
0ed5e12
Compare
because trans->is_closing is heavily accessed during account balance calculations, and it's reasonable to cache the kvp value.
0ed5e12
to
3ab5150
Compare
Where's the profiling data showing that a) there's a problem b) that caching improves it, and c) that caching is better than the |
See first comment which is the only profiling I've done. |
Either I need a bigger file or you need a faster computer. ;-) With a 54MB (uncompressed) book, average of the last 4 of 5 runs each (because the first run was several hundred ms longer in both cases):
Not exactly compelling. BTW, profiling with Xcode instruments shows 75% of the time is loading the db. The report takes only 388 ms. |
I've done more profiling, and the modes are still 5.5s without caching, and 5.1s with caching. |
If you're always getting the same times it means that you're always running the same code on the same data file. That I got different results is probably down to my having a different data file and might have to do that it doesn't have any closing transactions because I never close the book in that file. Mind, even your 9% improvement isn't exactly compelling. Running What you get when you use a profiler is an extended stack trace that shows you how long the program spent in each function of each execution branch. That enables you to quickly identify problem functions--or recognize that there aren't any problem functions and that what you're asking the computer to do just takes that long. That might lead you to reconsider the design to see if you can get the same result with less work; see my comment on loading the file being the dominant time-user. IIRC it's because xaccAccountCommitEdit is sorting its splits after every transaction insertion. Sorting calls xaccTransOrder and one of the things it orders on is isClosingTrans. Some observations: An obvious way to speed that up is calling xaccAccountBeginEdit on every account before starting to load the transactions, but while our XML files are laid out with the transactions last--and have to be because the accounts have to all be created before the transactions can be--there's no way for a SAX parser to know that. The SQL backend knows what in order it's lloading objects so it can do those optimizations. That aside, caching KVP merely works around GValue's inefficiency. You've already written an actual fix that gets rid of GValue. Use it. It's a maintainability improvement that doesn't need profiling for support. |
From my understanding the xaccAccountBegin/CommitEdit is already happening... maybe in
I'll see about porting the GValue->kvp to transaction.cpp functions, but I feel it's more of a code prettify than CPU optimization exercise. IIUC GValue is a simple struct with a datatype and union data see https://github.com/GNOME/glib/blob/f1a498e178709dcd5c26303a5b54ae5007733fc5/gobject/gvalue.h#L113 |
OK, that's good.
Right, a discriminated or tagged union. It still has to be initialized, have a value copied in, have its type tested, and the value copied out. The other time waster here is resolving the KVP path. We should be able to move that to compile time with some |
because
trans->is_closing
is heavily accessed during account balance calculations, therefore it's reasonable to cache the kvp value.was removed in 8a7c539
benchmarks improve from 5.5s to 5.0s