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

Add CachingBackend, address performance of JDBCBackend.item_get_elements #213

Merged
merged 13 commits into from
Nov 22, 2019

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Nov 21, 2019

This PR:

In the longer run, we will need to add performance tests (e.g. using pytest-benchmark) to ensure that the ixmp test suite directly tests that ixmp has satisfactory performance for data on the scale of MESSAGE-GLOBIOM (~10 dimensions, ~10⁶ rows). That will be a separate issue.

How to review

  • Read the code!
  • Try to load large parameters and ensure that performance is better when the cache is hit.
    • Import the code and use a custom script to load some large MESSAGE-GLOBIOM parameters.
    • Change N_dim in test_report_size to 8 and run only that test with pytest -k report_size.

PR checklist

  • Tests added.
  • Documentation added.
  • Release notes updated.

@khaeru khaeru changed the title Add CachingBackend Add CachingBackend, address performance of JDBCBackend.item_get_elements Nov 21, 2019
@khaeru khaeru self-assigned this Nov 22, 2019
@khaeru
Copy link
Member Author

khaeru commented Nov 22, 2019

@OFR-IIASA notes that this change brings the time to load the land_out MESSAGE-GLOBIOM parameter down from 470 seconds to 50 seconds, which is “reasonable”. This variable has about 1.9 million rows, so the benchmark is roughly 26 µs/row.

He also reports that the caching appears to work.

@khaeru khaeru requested review from zikolach and OFR-IIASA November 22, 2019 10:30
@khaeru khaeru mentioned this pull request Nov 22, 2019
Copy link

@OFR-IIASA OFR-IIASA left a comment

Choose a reason for hiding this comment

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

Code looks fine; I ran the tests and additional tests of my own to check that the speed of retrieving data as well as caching works as desired - all OK.

@khaeru khaeru added this to the 2.0 milestone Nov 22, 2019
@khaeru khaeru merged commit f68bbeb into iiasa:master Nov 22, 2019
@khaeru khaeru deleted the caching-backend branch November 22, 2019 12:18
Copy link
Contributor

@zikolach zikolach left a comment

Choose a reason for hiding this comment

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

Looks good to me. Would be great to compare memory allocation after switching to categorical dtypes. Thanks @khaeru!

@khaeru khaeru mentioned this pull request Jan 8, 2024
3 tasks
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.

3 participants