rust-rrule icon indicating copy to clipboard operation
rust-rrule copied to clipboard

simplifications and out-of-memory fix in RRuleIterSet

Open oll3 opened this issue 1 year ago • 1 comments

...and another one :1234:

Did some simplifications to the RRuleSetIter, mainly trying to get rid of the HashMap queue thing.

Following to this change it seemed possible to stop appending exrules dates to the exdates btree indefinitely (which eventually may cause out of memory on the system). And instead keeping track of the last value of each iterator. A change made possible since the date given to the is_date_excluded should now always be greater (or the same) as the last call to the function. Hence there is no need to track previously yielded exrule dates.

These changes seems to improve the overall iteration performance of the RRuleSetIter. I have created some benchmarks (not in PR) and the iteration time is cut down by 4-70 % depending on the specific rruleset tested.

oll3 avatar Jun 23 '24 23:06 oll3

To clarify this PR a bit more...

I wanted to remove this hashmap since it seemed to have an impact on the performance of the rruleset iterator. Hashing values may be expensive compute wise and is done in a quite tight loop. And it seems to be done at least once every loop depending on how complex the rruleset is.

When I understood the inner workings of the set iterator better it seemed this could be done an simplified way by using peekable iterators for the inner RRuleIters. This change decreased the code of the set iterator a bit (see commit f9aed0b (1 file changed, 59 insertions(+), 149 deletions(-)) while still outputting the same result. At least according to the current tests suite and how I understood the previous code.

This also made the inner iterators to always output an increasing date. Which, in turn, this made it possible to remove the btree storage of old exrules dates and fix this "leak".

To give an idea of the performance improvement, here is the output from running my benchmarks locally (sorry for the bad resolution). First run on the main branch (commit 8402921) then again on this branch (commit 4e0de2d4): image

The benchmark code is available here if you want to try or have any feedback.

I have also noticed that there are more room for improvements performance wise and already have a bunch of changes queued up. Personally I would really like to see these merged to this project. I have no problem adjusting or providing more tests if wanted. I understood that you (@fmeringdal) might value mature code higher, and that's of course ok. Still I'm wondering if this work is of any interest to this project or am I better of forking (is that ok?)?

oll3 avatar Jul 10 '24 12:07 oll3