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

Optimize cron #4010

Merged
merged 6 commits into from
Dec 5, 2024
Merged

Optimize cron #4010

merged 6 commits into from
Dec 5, 2024

Conversation

fubhy
Copy link
Member

@fubhy fubhy commented Nov 27, 2024

This should massively improve the efficiency (number of loops) for the Cron.next lookup. With this, I think we should also be able to support seconds & milisecond granularity.

That said, it's already possible to compose schedules with second or milisecond ticks within an active Cron interval... So I'm not sure how valuable that'd be tbh.

Closes #3932

Copy link

changeset-bot bot commented Nov 27, 2024

🦋 Changeset detected

Latest commit: 0796582

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 34 packages
Name Type
effect Patch
@effect/cli Patch
@effect/cluster-browser Patch
@effect/cluster-node Patch
@effect/cluster-workflow Patch
@effect/cluster Patch
@effect/experimental Patch
@effect/opentelemetry Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-node-shared Patch
@effect/platform-node Patch
@effect/platform Patch
@effect/printer-ansi Patch
@effect/printer Patch
@effect/rpc-http Patch
@effect/rpc Patch
@effect/sql-clickhouse Patch
@effect/sql-d1 Patch
@effect/sql-drizzle Patch
@effect/sql-kysely Patch
@effect/sql-libsql Patch
@effect/sql-mssql Patch
@effect/sql-mysql2 Patch
@effect/sql-pg Patch
@effect/sql-sqlite-bun Patch
@effect/sql-sqlite-node Patch
@effect/sql-sqlite-react-native Patch
@effect/sql-sqlite-wasm Patch
@effect/sql Patch
@effect/typeclass Patch
@effect/vitest Patch
@effect/ai Patch
@effect/ai-openai Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot changed the base branch from main to next-minor November 27, 2024 13:10
@fubhy fubhy force-pushed the optimize-cron branch 8 times, most recently from d3ed5dc to c2eb763 Compare November 27, 2024 18:19
@fubhy
Copy link
Member Author

fubhy commented Nov 27, 2024

@IMax153 has agreed to review this.

Screenshot 2024-11-27 at 19 45 23

@tim-smart
Copy link
Member

I think some more tests that cover off daylight savings, leap years etc. would be good.

@IMax153
Copy link
Member

IMax153 commented Nov 27, 2024

What Tim said :)

@github-actions github-actions bot force-pushed the next-minor branch 12 times, most recently from c1f32a7 to 0d4aa4f Compare November 30, 2024 23:07
@github-actions github-actions bot force-pushed the next-minor branch 6 times, most recently from e9c8ce4 to f47d935 Compare December 1, 2024 22:38
@fubhy fubhy changed the base branch from next-minor to main December 2, 2024 09:45
@fubhy fubhy removed the next-minor label Dec 2, 2024
@fubhy
Copy link
Member Author

fubhy commented Dec 2, 2024

I think some more tests that cover off daylight savings, leap years etc. would be good.

Can you verify if this makes any sense now?

After looking through various Cron implementations, I believe this is the simplest we can do whilst ticking "most" boxes. The only downside is that we skip the transition period when moving out of daylight savings time. I believe that's fine and after experimenting with all sorts of different Cron implementations, they all handle this differently in their own way (some repeat the same hour twice, some skip it like we do, some are outright bugged). I haven't found a way to elegantly solve this without adding complexity that is (imho) not waranted here.

If we feel strongly that we also want to fully cover the transition period coming out of daylight savings time, we can address that in a follow-up imho.

wdyt @tim-smart @IMax153 ?

@fubhy fubhy requested a review from tim-smart December 4, 2024 11:48
@tim-smart tim-smart merged commit 6e323a3 into main Dec 5, 2024
12 checks passed
@tim-smart tim-smart deleted the optimize-cron branch December 5, 2024 21:26
@github-actions github-actions bot mentioned this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Optimize Cron.next algorithm
3 participants