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

GetNextIncludedTimeUtc method is very slow or crashes #2270

Open
al007 opened this issue Feb 5, 2024 · 8 comments
Open

GetNextIncludedTimeUtc method is very slow or crashes #2270

al007 opened this issue Feb 5, 2024 · 8 comments

Comments

@al007
Copy link

al007 commented Feb 5, 2024

The GetNextIncludedTimeUtc method of ICalendar can be extremely slow in certain conditions. The below example creates a calendar that excludes public holidays, weekends, and nighttime. A call to the GetNextIncludedTimeUtc method of such a calendar takes more than 13 seconds on my machine.

Tested version Quartz 3.8.0 from Nuget as well as the latest version of the main branch of this repo.

[Test]
public void GetNextIncludedTimeUtc()
{
    TimeZoneInfo timeZone = TimeZoneInfo.FindSystemTimeZoneById("Pacific Standard Time");

    var holidayCalendar = new HolidayCalendar()
    {
        TimeZone = timeZone,
    };
    holidayCalendar.AddExcludedDate(new DateTime(2024, 2, 19));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 5, 27));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 6, 19));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 7, 4));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 9, 2));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 10, 14));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 11, 11));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 11, 28));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 12, 25));

    var weeklyCalendar = new WeeklyCalendar(holidayCalendar)
    {
        TimeZone = timeZone,
    };
    var calendar = new DailyCalendar(weeklyCalendar, "06:00", "22:00")
    {
        TimeZone = timeZone,
        InvertTimeRange = true,
    };
    var time = new DateTime(2024, 2, 5, 10, 6, 0, DateTimeKind.Utc);
    var expected = new DateTime(2024, 2, 5, 14, 0, 0, DateTimeKind.Utc);

    var d = calendar.GetNextIncludedTimeUtc(time); // takes 13 seconds to execute!
    d.Should().Be(expected);
}
@al007
Copy link
Author

al007 commented Feb 5, 2024

The following calendar configuration crashes on GetNextIncludedTimeUtc: System.ArgumentOutOfRangeException : The UTC time represented when the offset is applied must be between year 0 and 10,000. (Parameter 'offset')

    [Test]
    public void GetNextIncludedTimeUtc()
    {
        TimeZoneInfo timeZone = TimeZoneInfo.FindSystemTimeZoneById("Pacific Standard Time");

        var weeklyCalendar = new WeeklyCalendar()
        {
            TimeZone = timeZone,
        };

        var dailyCalendar = new DailyCalendar(weeklyCalendar, "06:00", "22:00")
        {
            TimeZone = timeZone,
            InvertTimeRange = true,
        };

        var holidayCalendar = new HolidayCalendar(dailyCalendar)
        {
            TimeZone = timeZone,
        };
        holidayCalendar.AddExcludedDate(new DateTime(2024, 2, 19));
        holidayCalendar.AddExcludedDate(new DateTime(2024, 5, 27));
        holidayCalendar.AddExcludedDate(new DateTime(2024, 6, 19));
        holidayCalendar.AddExcludedDate(new DateTime(2024, 7, 4));
        holidayCalendar.AddExcludedDate(new DateTime(2024, 9, 2));
        holidayCalendar.AddExcludedDate(new DateTime(2024, 10, 14));
        holidayCalendar.AddExcludedDate(new DateTime(2024, 11, 11));
        holidayCalendar.AddExcludedDate(new DateTime(2024, 11, 28));
        holidayCalendar.AddExcludedDate(new DateTime(2024, 12, 25));

        var time = new DateTime(2024, 2, 5, 10, 6, 0, DateTimeKind.Utc);
        var expected = new DateTime(2024, 2, 5, 14, 0, 0, DateTimeKind.Utc);

        var d = holidayCalendar.GetNextIncludedTimeUtc(time); // throws System.ArgumentOutOfRangeException : The UTC time represented when the offset is applied must be between year 0 and 10,000. (Parameter 'offset')
        d.Should().Be(expected);
    }

@al007 al007 changed the title GetNextIncludedTimeUtc method is very slow GetNextIncludedTimeUtc method is very slow or crashes Feb 5, 2024
@jafin
Copy link
Contributor

jafin commented Feb 5, 2024

Possibly related dotnet/runtime#25075
One hotspot is the calls to TimeZoneInfo.ConvertTime in a tight loop.

image

@lahma
Copy link
Member

lahma commented Feb 11, 2024

The problem here is that DailyCalendar.GetNextIncludedTimeUtc() iterates checking every next millisecond whether it would fit the range. Increments could be larger / start position could be calculated more precisely when we know that there's no need for millisecond or even second precision.

jafin added a commit to jafin/quartznet that referenced this issue Feb 23, 2024
…o checking modified DateOnly value with base.

Changed logic to compare original value with base Calendars, and Date only value with current validator.
Add scenario described in quartznet#2270
jafin added a commit to jafin/quartznet that referenced this issue Feb 23, 2024
…o checking modified DateOnly value with base.

Changed logic to compare original value with base Calendars, and Date only value with current validator.
Add scenario described in quartznet#2270
jafin added a commit to jafin/quartznet that referenced this issue Feb 23, 2024
…o checking modified DateOnly value with base.

Changed logic to compare original value with base Calendars, and Date only value with current validator.
Add scenario described in quartznet#2270
jafin added a commit to jafin/quartznet that referenced this issue Feb 23, 2024
lahma pushed a commit that referenced this issue Feb 24, 2024
… NextIncludedTime (#2284)

Changed logic to compare original value with base Calendars, and Date only value with current validator.
Add scenario described in #2270
@jafin
Copy link
Contributor

jafin commented Feb 27, 2024

@al007 Please check if latest 3.x / master branches in repo resolves your issue.

@al007
Copy link
Author

al007 commented Mar 4, 2024

No, unfortunately, it does not. The example from the 1st post in this thread works even slower now: 20 seconds for .net8 and 50 seconds for net472. However, it does fix the crash for the 2nd example.

@jafin
Copy link
Contributor

jafin commented Mar 4, 2024

The example you provided should be in the repo, it executes here in about 43ms.
This is from 3.x branch.

Can you please try running.?

dotnet test --filter GetNextIncludedTimeUtc_CrashOriginal2270
Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: 43 ms - Quartz.Tests.Unit.dll (net8.0)

@al007
Copy link
Author

al007 commented Mar 4, 2024

@jafin , this method has two issues: poor performance and a crash. I reported them together because I thought they were related and re-working the algorithm would fix both.

Performance issue happens for the following calendar configuration HolidayCalendar->WeeklyCalendar->DailyCalendar, see 1st post here. It is not fixed in the 3.x branch. It takes from 20 to 50 seconds as described above.

The crash happens for calendar WeeklyCalendar->DailyCalendar->HolidayCalendar, see 2nd post here. It is fixed in the 3.x branch and runs fast.

@jafin
Copy link
Contributor

jafin commented Mar 4, 2024

@al007 Perhaps I am missing something I copied the example, ran the below against 3.x branch, completed in ~20ms

Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: 11 ms - Quartz.Tests.Unit.dll (net8.0)
Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: 22 ms - Quartz.Tests.Unit.dll (net472)
[Test]
public void GetNextIncludedTimeUtcFirstIssue()
{
    TimeZoneInfo timeZone = TimeZoneInfo.FindSystemTimeZoneById("Pacific Standard Time");

    var holidayCalendar = new HolidayCalendar()
    {
        TimeZone = timeZone,
    };
    holidayCalendar.AddExcludedDate(new DateTime(2024, 2, 19));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 5, 27));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 6, 19));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 7, 4));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 9, 2));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 10, 14));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 11, 11));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 11, 28));
    holidayCalendar.AddExcludedDate(new DateTime(2024, 12, 25));

    var weeklyCalendar = new WeeklyCalendar(holidayCalendar)
    {
        TimeZone = timeZone,
    };
    var calendar = new DailyCalendar(weeklyCalendar, "06:00", "22:00")
    {
        TimeZone = timeZone,
        InvertTimeRange = true,
    };
    var time = new DateTime(2024, 2, 5, 10, 6, 0, DateTimeKind.Utc);
    var expected = new DateTime(2024, 2, 5, 14, 0, 0, DateTimeKind.Utc);

    var d = calendar.GetNextIncludedTimeUtc(time); // takes 13 seconds to execute!
    d.Should().Be(expected);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants