Replace `DateTime` with `CalDateTime` in `RecurrencePatternEvaluator` and related code #742

axunonb · 2025-03-02T09:46:52Z

Replace DateTime with CalDateTime in RecurrencePatternEvaluator and related code. Key changes:

Modifying the Until property of RecurrencePattern to use CalDateTime.
Updating ExceptionDates to directly add CalDateTime instances.
Refactoring methods like GetIso8601WeekOfYear to accept CalDateTime parameters.
Adjusting serialization and deserialization processes for CalDateTime.
Adjusting the RecurrencePatternEvaluator to work with CalDateTime.

axunonb · 2025-03-02T09:54:06Z

Here are the remaining members of Ical.Net that have a DateTime parameter or return type.
@minichma TBD: Any of them that should be updated, too? (Expecially considering GetOccurrences)

Other:

Replace instances of `DateTime` with `CalDateTime` across the codebase. Key updates include: - Remove the `CalDateTime implicit operator` for converstion of `DateTime` to `CalDateTime` - Modifying the Until property of `RecurrencePattern` to use `CalDateTime`. - Updating `ExceptionDates` to directly add `CalDateTime` instances. - Refactoring methods like `GetIso8601WeekOfYear` to accept `CalDateTime` parameters. - Adjusting serialization and deserialization processes for `CalDateTime`. - Adjusting the `RecurrencePatternEvaluator` to work with `CalDateTime`.

minichma · 2025-03-04T08:21:30Z

@axunonb

TBD: Any of them that should be updated, too? (Expecially considering GetOccurrences)

I'd say ideally most of them, especially the GetOccurrences, except maybe some internal ones and those intended for conversion to/from DateTime.

axunonb · 2025-03-04T08:58:30Z

Gonna create another PR for GetOccurrences updates.

Ical.Net/CalendarExtensions.cs

minichma · 2025-03-04T09:08:16Z

Ical.Net/CalendarExtensions.cs

    /// in, according to the week numbering required by RFC 5545.
    /// </summary>
-    private static DateTime GetStartOfWeek(this DateTime t, DayOfWeek firstDayOfWeek)
+    private static CalDateTime GetStartOfWeek(this CalDateTime t, DayOfWeek firstDayOfWeek)


Could also be DateOnly

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

axunonb · 2025-03-04T11:27:06Z

Maybe check the performance impact of changing the internals of this class to CalDateTime. Could be measurable.

Yes, true, quite significant... You think the price is too high?

v4.3.1

// * Summary *

BenchmarkDotNet v0.14.0, Windows 11 (10.0.26100.3194)
13th Gen Intel Core i7-13700K, 1 CPU, 24 logical and 16 physical cores
.NET SDK 9.0.200
  [Host] : .NET 8.0.13 (8.0.1325.6609), X64 RyuJIT AVX2

Toolchain=InProcessNoEmitToolchain

| Method            | Mean         | Error      | StdDev     | Gen0     | Gen1     | Allocated   |
|------------------ |-------------:|-----------:|-----------:|---------:|---------:|------------:|
| SerializeCalendar |     6.222 us |  0.0748 us |  0.0700 us |   1.2207 |        - |    18.99 KB |
| GetOccurrences    | 4,017.977 us | 46.3923 us | 43.3953 us | 679.6875 | 515.6250 | 10426.99 KB |

// * Hints *
Outliers
  RecurrenceBenchmark.SerializeCalendar: Toolchain=InProcessNoEmitToolchain -> 2 outliers were detected (6.15 us, 6.19 us)
  RecurrenceBenchmark.GetOccurrences: Toolchain=InProcessNoEmitToolchain    -> 2 outliers were detected (1.31 ms, 1.32 ms)

// * Legends *
  Mean      : Arithmetic mean of all measurements
  Error     : Half of 99.9% confidence interval
  StdDev    : Standard deviation of all measurements
  Gen0      : GC Generation 0 collects per 1000 operations
  Gen1      : GC Generation 1 collects per 1000 operations
  Allocated : Allocated memory per single operation (managed only, inclusive, 1KB = 1024B)
  1 us      : 1 Microsecond (0.000001 sec)

// * Diagnostic Output - MemoryDiagnoser *


// ***** BenchmarkRunner: End *****
Run time: 00:00:23 (23.44 sec), executed benchmarks: 2

Global total time: 00:00:23 (23.81 sec), executed benchmarks: 2

Current Main

// * Summary *

BenchmarkDotNet v0.14.0, Windows 11 (10.0.26100.3194)
13th Gen Intel Core i7-13700K, 1 CPU, 24 logical and 16 physical cores
.NET SDK 9.0.200
  [Host] : .NET 8.0.13 (8.0.1325.6609), X64 RyuJIT AVX2

Toolchain=InProcessNoEmitToolchain

| Method            | Mean       | Error     | StdDev    | Gen0     | Gen1     | Allocated |
|------------------ |-----------:|----------:|----------:|---------:|---------:|----------:|
| SerializeCalendar |   6.318 us | 0.0514 us | 0.0456 us |   1.4038 |   0.0229 |  21.61 KB |
| GetOccurrences    | 660.939 us | 2.7257 us | 2.5496 us | 143.5547 | 113.2813 |   2210 KB |

// * Hints *
Outliers
  RecurrenceBenchmark.SerializeCalendar: Toolchain=InProcessNoEmitToolchain -> 1 outlier  was  removed (6.58 us)

// * Legends *
  Mean      : Arithmetic mean of all measurements
  Error     : Half of 99.9% confidence interval
  StdDev    : Standard deviation of all measurements
  Gen0      : GC Generation 0 collects per 1000 operations
  Gen1      : GC Generation 1 collects per 1000 operations
  Allocated : Allocated memory per single operation (managed only, inclusive, 1KB = 1024B)
  1 us      : 1 Microsecond (0.000001 sec)

// * Diagnostic Output - MemoryDiagnoser *


// ***** BenchmarkRunner: End *****
Run time: 00:00:39 (39.13 sec), executed benchmarks: 2

Global total time: 00:00:39 (39.52 sec), executed benchmarks: 2

PR #742

// * Summary *

BenchmarkDotNet v0.14.0, Windows 11 (10.0.26100.3194)
13th Gen Intel Core i7-13700K, 1 CPU, 24 logical and 16 physical cores
.NET SDK 9.0.200
  [Host] : .NET 8.0.13 (8.0.1325.6609), X64 RyuJIT AVX2

Toolchain=InProcessNoEmitToolchain

| Method            | Mean       | Error     | StdDev    | Gen0     | Gen1    | Allocated  |
|------------------ |-----------:|----------:|----------:|---------:|--------:|-----------:|
| SerializeCalendar |   6.261 us | 0.0405 us | 0.0379 us |   1.4038 |  0.0229 |   21.61 KB |
| GetOccurrences    | 829.253 us | 2.2819 us | 1.7816 us | 156.2500 | 94.7266 | 2397.47 KB |

// * Hints *
Outliers
  RecurrenceBenchmark.GetOccurrences: Toolchain=InProcessNoEmitToolchain -> 3 outliers were removed, 5 outliers were detected (825.28 us, 826.41 us, 833.97 us..835.15 us)

// * Legends *
  Mean      : Arithmetic mean of all measurements
  Error     : Half of 99.9% confidence interval
  StdDev    : Standard deviation of all measurements
  Gen0      : GC Generation 0 collects per 1000 operations
  Gen1      : GC Generation 1 collects per 1000 operations
  Allocated : Allocated memory per single operation (managed only, inclusive, 1KB = 1024B)
  1 us      : 1 Microsecond (0.000001 sec)

// * Diagnostic Output - MemoryDiagnoser *


// ***** BenchmarkRunner: End *****
Run time: 00:00:43 (43.06 sec), executed benchmarks: 2

Global total time: 00:00:43 (43.44 sec), executed benchmarks: 2

Benchmark

[MemoryDiagnoser]
public class RecurrenceBenchmark
{
    private Calendar _calendar;

    [GlobalSetup]
    public void Setup()
    {
        _calendar = GenerateCalendarWithRecurrences();
    }

    private Calendar GenerateCalendarWithRecurrences()
    {
        var calendar = new Calendar();
        var dailyEvent = new CalendarEvent
        {
            Start = new CalDateTime(2025, 3, 1),
            End = null,
            RecurrenceRules = new List<RecurrencePattern>
            {
                new RecurrencePattern(FrequencyType.Daily, 1) { Count = 1000 }
            }
        };
        calendar.Events.Add(dailyEvent);
        return calendar;
    }

    [Benchmark]
    public void SerializeCalendar()
    {
        var serializer = new CalendarSerializer();
        var serializedCalendar = serializer.SerializeToString(_calendar);
    }

    [Benchmark]
    public void GetOccurrences()
    {
        var occurrences = _calendar.GetOccurrences().ToList();
    }
}

minichma · 2025-03-04T11:42:46Z

The problem with CalDateTime is that its a class while DateTime, DateOnly, etc. are structs. So for all the little operations in RecurrenceEvaluator, where tons of objects are allocated, this makes a difference. But the difference is lower than I'd have expected, so I wouldn't expect many complaints from users either way. You could either revert the change just for the internals of RecurrencePatternEvaluator (other classes won't be affected too much), or just leave it as is. Both is ok from my perspective. At some point we might either change CalDateTime to struct or move evaluation to different types as discussed in other threads. ZonedDateTime et al for instance are structs.

axunonb · 2025-03-04T21:26:09Z

@minichma I was actually hesitating decreasing performance.
Now I added the same benchmark test against v4.3.1, because I was confident that the new IEnumerable for occurrences had to be a lot better than the v4.3.1 HashSet. And it was, even when using CalDateTime, kudos to your effort.
So finally we should proceed (with a clear conscience) as you suggested.

minichma · 2025-03-05T06:25:45Z

@axunonb Great, always good to be confident by measuring things!

sonarqubecloud · 2025-03-05T08:08:36Z

Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

axunonb force-pushed the pr/datetime-to-caldatetime branch from 4af837a to 20323c8 Compare March 2, 2025 09:51

axunonb force-pushed the pr/datetime-to-caldatetime branch 3 times, most recently from 41526ad to b03ffe5 Compare March 2, 2025 11:47

axunonb marked this pull request as draft March 2, 2025 16:56

axunonb force-pushed the pr/datetime-to-caldatetime branch from b03ffe5 to 2e78621 Compare March 4, 2025 07:16

axunonb force-pushed the pr/datetime-to-caldatetime branch from 2e78621 to dda621a Compare March 4, 2025 07:19

axunonb changed the title ~~Refactor date-time handling to use CalDateTime instead of DateTime~~ Replace DateTime with CalDateTime in CalDateTime in RecurrencePatternEvaluator and related code Mar 4, 2025

axunonb marked this pull request as ready for review March 4, 2025 08:59

axunonb requested a review from minichma March 4, 2025 08:59

axunonb changed the title ~~Replace DateTime with CalDateTime in CalDateTime in RecurrencePatternEvaluator and related code~~ Replace DateTime with CalDateTime RecurrencePatternEvaluator and related code Mar 4, 2025

axunonb changed the title ~~Replace DateTime with CalDateTime RecurrencePatternEvaluator and related code~~ Replace DateTime with CalDateTime in RecurrencePatternEvaluator and related code Mar 4, 2025

minichma approved these changes Mar 4, 2025

View reviewed changes

Merge branch 'main' into pr/datetime-to-caldatetime

a30c3bb

axunonb merged commit a0db138 into ical-org:main Mar 5, 2025
3 of 4 checks passed

axunonb deleted the pr/datetime-to-caldatetime branch March 5, 2025 08:11

This was referenced Jul 28, 2025

Bump the all-updates group with 4 updates mikkelkaas/HaandboldGrabber#13

Closed

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#243

Closed

dependabot bot mentioned this pull request Aug 4, 2025

Bump Ical.Net and 5 others Atrejoe/Inky-Calendar-Server#245

Closed

dependabot bot mentioned this pull request Aug 18, 2025

Bump the all-updates group with 4 updates mikkelkaas/HaandboldGrabber#14

Merged

dependabot bot mentioned this pull request Aug 25, 2025

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#246

Closed

dependabot bot mentioned this pull request Sep 1, 2025

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#253

Closed

dependabot bot mentioned this pull request Oct 13, 2025

Bump Ical.Net from 4.3.1 to 5.1.1 Atrejoe/Inky-Calendar-Server#257

Open

This was referenced Oct 23, 2025

Bump Ical.Net from 4.3.1 to 5.1.1 joinrpg/joinrpg-net#3675

Closed

Bump Ical.Net from 4.3.1 to 5.1.1 loresoft/InstructorIQ#379

Merged

Bump Ical.Net from 4.3.1 to 5.1.1 joinrpg/joinrpg-net#3707

Open

Replace DateTime with CalDateTime in RecurrencePatternEvaluator and related code #742

Replace DateTime with CalDateTime in RecurrencePatternEvaluator and related code #742

Uh oh!

Conversation

axunonb commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axunonb commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minichma commented Mar 4, 2025

Uh oh!

axunonb commented Mar 4, 2025

Uh oh!

Uh oh!

Uh oh!

minichma Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

axunonb Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

axunonb commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

v4.3.1

Current Main

PR #742

Benchmark

Uh oh!

minichma commented Mar 4, 2025

Uh oh!

axunonb commented Mar 4, 2025

Uh oh!

minichma commented Mar 5, 2025

Uh oh!

sonarqubecloud bot commented Mar 5, 2025

Quality Gate failed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Replace `DateTime` with `CalDateTime` in `RecurrencePatternEvaluator` and related code #742

Replace `DateTime` with `CalDateTime` in `RecurrencePatternEvaluator` and related code #742

axunonb commented Mar 2, 2025 •

edited

Loading

axunonb commented Mar 2, 2025 •

edited

Loading

axunonb commented Mar 4, 2025 •

edited

Loading