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

Fix performance issue with reading time series #410

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Fix performance issue with reading time series #410

merged 2 commits into from
Oct 31, 2024

Conversation

daniel-thom
Copy link
Contributor

@daniel-thom daniel-thom commented Oct 30, 2024

Three problems are exposed by #409:

  • PowerSimulations was retrieving time series UUIDs through calls to the InfrastructureSystems database when those UUIDs were already cached. That is addressed in Use cached time series UUID PowerSimulations.jl#1169
  • The issue above showed that calls to Base.root_module during deserialization are too slow. This PR caches values to avoid unnecessary latency.
  • Reading SingleTimeSeries arrays was unnecessarily checking for consistent resolution. That is already checked when the arrays are added to the system. This PR removes those unnecessary checks.

There are still some performance issues related to retrieval of time series metadata that will be fixed in future work.

@daniel-thom daniel-thom marked this pull request as draft October 30, 2024 21:56
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.75%. Comparing base (8c4d57d) to head (d7069b7).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/utils/utils.jl 91.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
+ Coverage   77.65%   77.75%   +0.10%     
==========================================
  Files          69       69              
  Lines        5474     5481       +7     
==========================================
+ Hits         4251     4262      +11     
+ Misses       1223     1219       -4     
Flag Coverage Δ
unittests 77.75% <91.66%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/single_time_series.jl 70.47% <ø> (-0.17%) ⬇️
src/utils/utils.jl 64.68% <91.66%> (+2.22%) ⬆️

... and 1 file with indirect coverage changes

@daniel-thom daniel-thom force-pushed the issue-409 branch 2 times, most recently from b687886 to a19168a Compare October 30, 2024 23:42
The code was unnecessarily checking the consistency of a single time
series resolution in calls to get_time_series. The check occurs when
the array is added to the system.
@daniel-thom daniel-thom changed the title Issue 409 Fix performance issue with reading time series Oct 31, 2024
@daniel-thom daniel-thom requested a review from jd-lara October 31, 2024 00:54
@daniel-thom daniel-thom marked this pull request as ready for review October 31, 2024 00:54
@jd-lara jd-lara merged commit 76c1d77 into main Oct 31, 2024
10 checks passed
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

Successfully merging this pull request may close these issues.

2 participants