Skip to content

feat: Only initialize BusinessCalendar caches when data is accessed and improve caching performance#5378

Merged
chipkent merged 57 commits intodeephaven:mainfrom
chipkent:bus_cal_init_speed
Oct 23, 2024
Merged

feat: Only initialize BusinessCalendar caches when data is accessed and improve caching performance#5378
chipkent merged 57 commits intodeephaven:mainfrom
chipkent:bus_cal_init_speed

Conversation

@chipkent
Copy link
Copy Markdown
Member

@chipkent chipkent commented Apr 17, 2024 •
edited
Loading

Makes BusinessCalendar caches populate only when accessed to avoid slow worker initialization. Additionally, caching in various operations has been improved to yield better performance.

Resolves #5377

Benchmark:

n = 1_000_000
npart = 10
t = emptyTable(n)
        .updateView("Part = ii%npart", "Today = todayLocalDate()")

tp = t.partitionBy("Part").proxy()

c = calendar()

timeit = { name, table, query ->
    c.clearCache()
    start = System.currentTimeMillis()
    rst = table.update(query)
    end = System.currentTimeMillis()
    println("TIME: ${name}: ${end-start} ms")
}

istart = parseInstant("2022-01-01T01:01:01 ET")
iend = parseInstant("2024-06-01T01:01:01 ET")

timeit("calendarDayOld-NORMAL", t, "CD = c.calendarDay(Today)")
timeit("calendarDayOld-PART", tp, "CD = c.calendarDay(Today)")

timeit("diffBusinessYearsOld-NORMAL", t, "CD = c.diffBusinessYears(istart, iend)")
timeit("diffBusinessYearsOld-PART", tp, "CD = c.diffBusinessYears(istart, iend)")

println("DONE")

Performance:

// 1M - NEW
//TIME: calendarDayOld-NORMAL: 266 ms
//TIME: calendarDayOld-PART: 299 ms
//TIME: diffBusinessYearsOld-NORMAL: 5937 ms
//TIME: diffBusinessYearsOld-PART: 6664 ms <<<<

// 1M - v0.33.3
//TIME: calendarDayOld-NORMAL: 366 ms
//TIME: calendarDayOld-PART: 327 ms
//TIME: diffBusinessYearsOld-NORMAL: 20384 ms
//TIME: diffBusinessYearsOld-PART: 5549 ms <<<<

// 10M - NEW
//TIME: calendarDayOld-NORMAL: 449 ms
//TIME: calendarDayOld-PART: 548 ms
//TIME: diffBusinessYearsOld-NORMAL: 58883 ms
//TIME: diffBusinessYearsOld-PART: 67362 ms <<<<

// 10M - v0.33.3
//TIME: calendarDayOld-NORMAL: 522 ms
//TIME: calendarDayOld-PART: 704 ms
//TIME: diffBusinessYearsOld-NORMAL: 195916 ms
//TIME: diffBusinessYearsOld-PART: 49411 ms <<<<

Note1: v0.33.0 times do not include initialization times, but the new values do.
Note2: There appears to be contention or initialization overhead in the partitioned table case yielding slightly worse performance.

@chipkent chipkent added this to the 1. March 2024 milestone Apr 17, 2024
@chipkent chipkent requested review from cpwright and rcaudy April 17, 2024 15:30
@chipkent chipkent self-assigned this Apr 17, 2024
@chipkent chipkent changed the title Only initialize BusinessCalendar caches when data is accessed Only initialize BusinessCalendar caches when data is accessed and improve caching performance Apr 18, 2024
@chipkent chipkent added NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. devrel-watch DevRel team is watching labels Apr 19, 2024
@chipkent chipkent requested a review from jjbrosnan April 19, 2024 04:35
jjbrosnan
jjbrosnan reviewed Apr 19, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@jjbrosnan jjbrosnan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any specific comments on the code. I was unable to break things when testing this locally. I tested against docs CI, and ran my own tests locally with no issues.

devinrsmith
devinrsmith reviewed Jun 18, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see #5378 (comment)

Comment thread engine/time/src/main/java/io/deephaven/time/calendar/CalendarDay.java Outdated
Comment thread engine/time/src/main/java/io/deephaven/time/calendar/CalendarDay.java Outdated
Comment thread engine/time/src/main/java/io/deephaven/time/calendar/Calendar.java Outdated
@chipkent chipkent mentioned this pull request Jun 21, 2024
Closed
@chipkent chipkent changed the title Only initialize BusinessCalendar caches when data is accessed and improve caching performance feat: Only initialize BusinessCalendar caches when data is accessed and improve caching performance Jun 24, 2024
@pete-petey pete-petey modified the milestones: 1. March 2024, 5. Backlog Aug 26, 2024
rcaudy
rcaudy reviewed Sep 24, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some trivial comments.

Comment thread engine/time/src/main/java/io/deephaven/time/calendar/ImmutableConcurrentCache.java Outdated
Comment thread engine/time/src/main/java/io/deephaven/time/calendar/ImmutableConcurrentCache.java Outdated
Comment thread engine/time/src/main/java/io/deephaven/time/calendar/ImmutableConcurrentCache.java Outdated
Comment thread engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java Outdated
Comment thread engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java Outdated
@chipkent chipkent requested a review from rcaudy October 3, 2024 22:09
rcaudy
rcaudy reviewed Oct 23, 2024
View reviewed changes
Comment thread engine/time/src/main/java/io/deephaven/time/calendar/ReadOptimizedConcurrentCache.java Outdated
@chipkent
Addressing review comments
e77fa84 
Minimizing the lifetime of recomputed values.
@chipkent chipkent requested a review from rcaudy October 23, 2024 19:23
rcaudy
rcaudy approved these changes Oct 23, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed enough, has great test coverage.

@chipkent chipkent enabled auto-merge (squash) October 23, 2024 19:27
@chipkent chipkent merged commit 1635218 into deephaven:main Oct 23, 2024
@chipkent chipkent deleted the bus_cal_init_speed branch October 23, 2024 19:53
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 23, 2024
@chipkent chipkent removed the devrel-watch DevRel team is watching label Oct 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rcaudy rcaudy rcaudy approved these changes

@cpwright cpwright Awaiting requested review from cpwright

@devinrsmith devinrsmith Awaiting requested review from devinrsmith

@jjbrosnan jjbrosnan Awaiting requested review from jjbrosnan

Assignees

@chipkent chipkent

Labels

March2024 NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. query engine

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

BusinessCalendar initialization is causing initialization delays

5 participants

@chipkent @rcaudy @devinrsmith @jjbrosnan @pete-petey
⚡