search

square / ruby-rrule

RRULE expansion for Ruby
Apache License 2.0
171 stars 25 forks source link

Optimize simple weekly case #7

Closed ordermuppet closed 6 years ago

ordermuppet commented 6 years ago

Using a different strategy for the simple weekly case with no additional filters results in a 40-50% faster expansion for those rules, when expanded over a short timespan of a month (I expect that the speedup would be even larger when expanding over long periods of time, as in those cases the time taken to expand is proportionally larger than the initialization time of parsing options, etc).

I think it makes sense to start with this case for now and gain some confidence that it doesn't miss any edge cases, but this strategy (identifying simple cases and handing them separately than the cases that require filters) might be a good one to apply here in general.

Not ready to merge this yet, as I want to update the specs for the frequencies if this approach looks good, but wanted to get a PR out to collect feedback.

ordermuppet commented 6 years ago

@yellow-beard Yep, for sure! Added a benchmark script and a file to track history. Let me know what you think.

leecunliffe commented 6 years ago

Looks good. Perhaps a note in the code to explain that this is for performance might be good too?

I'm a little concerned to see some of the specs now using UTC rather than a timezone that people actually live in / use. I know we've been bitten in the past by tests that worked in UTC, but misbehaved in actual timezones. In real world usage at Square, AFAIK, we're always expanding in a non-utc timezone.

ordermuppet commented 6 years ago

Yep that makes sense - updated the specs to use CA time, and added a short comment about why we'd use SimpleWeekly.