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

Speed improvements on Record init #658

Open
srfwx opened this issue Dec 4, 2024 · 6 comments · May be fixed by #659
Open

Speed improvements on Record init #658

srfwx opened this issue Dec 4, 2024 · 6 comments · May be fixed by #659
Labels
type: feature Introduction of new functionality to the application

Comments

@srfwx
Copy link
Contributor

srfwx commented Dec 4, 2024

pynetbox version

v7.4.1

NetBox version

v4.1.7

Feature type

Change to existing functionality

Proposed functionality

Hi,

We've built a caching layer to Pynetbox where responses are stored as json inside a Redis cache, then Record objects are reconstructed from there on cache hit.

However I've noticed that the _init_ of a Record object can still take a substantial amount of time due to the various processing steps

As a first step I have:

  • simplified the _endpoint_from_url method so that it doesn't need urlsplit leading to a 50% speed improvement of that method
  • used marshall dump/load instead of deep_copy for cache initialization
  • refactored the _parse_values method to limit the number of instance type checking and number of key/value that need further processing

All in all, according to a few benchmarks I made, the whole Record init shows more than 50% speed improvement. Below is an example when loading 14407 records.

Benchmark using cProfile with Pynetbox v7.4.1

      ncalls  tottime  percall  cumtime  percall filename:lineno(function)
142153/14407    0.829    0.000    8.681    0.001 .venv/lib/python3.10/site-packages/pynetbox/core/response.py:278(__init__)

Benchmark using cProfile with our fork

      ncalls  tottime  percall  cumtime  percall filename:lineno(function)
142153/14407    0.343    0.000    4.037    0.000 .venv/lib/python3.10/site-packages/pynetbox/core/response.py:263(__init__)

Would you be interested in having any/all of these things merged into upstream?

Use case

General speed improvement of the library

External dependencies

None

@srfwx srfwx added the type: feature Introduction of new functionality to the application label Dec 4, 2024
@srfwx
Copy link
Contributor Author

srfwx commented Dec 4, 2024

Latest benchmark after adding a cache of endpoints on App object and cache of apps on PluginsApp, reaching now a 70% speed improvement.

      ncalls  tottime  percall  cumtime  percall filename:lineno(function)
142153/14407    0.220    0.000    2.763    0.000  .venv/lib/python3.10/site-packages/pynetbox/core/response.py:263(__init__)

@arthanson
Copy link
Collaborator

@srfwx can you provide a PR for it so we can review it? It sounds beneficial but hard to say without seeing the exact code.

@srfwx srfwx linked a pull request Dec 4, 2024 that will close this issue
@srfwx
Copy link
Contributor Author

srfwx commented Dec 4, 2024

Hi,
Yes of course, here is the PR #659
I followed the contribution guidelines and opened an issue first.
Edit: last commit fix the tests

@srfwx
Copy link
Contributor Author

srfwx commented Dec 5, 2024

Further improvement achieved by only computing the Record.endpoint on demand and not during init. (0f9e41b)

      ncalls  tottime  percall  cumtime  percall filename:lineno(function)
142153/14407    0.200    0.000    2.305    0.000 .venv/lib/python3.10/site-packages/pynetbox/core/response.py:262(__init__)

@srfwx
Copy link
Contributor Author

srfwx commented Dec 5, 2024

I've noticed that nested Record are instantiated from scratch each time, although when dealing with a RecordSet you'll likely be encountering the same sub Record many times. Hence temporarily caching and re-using instead of processing again lead to even better performance. My benchmark is now showing a ~90% improvement

      ncalls  tottime  percall  cumtime  percall filename:lineno(function)
16409/14407    0.023    0.000    1.001    0.000 .venv/lib/python3.10/site-packages/pynetbox/core/response.py:261(__init__)

💡 This also gives the benefit that if you call .full_details() on any nested Record, then full details will be available in all other Records from the RecordSet with the same linked object

@srfwx srfwx changed the title Speed improvements on Record init Draft: Speed improvements on Record init Dec 6, 2024
@srfwx srfwx changed the title Draft: Speed improvements on Record init Speed improvements on Record init Dec 6, 2024
@srfwx
Copy link
Contributor Author

srfwx commented Dec 9, 2024

Nested dictionaries representing a Choice value are instantiated as nested Record with broken methods as they don't have an URL or endpoint.
With d3b1a8d I've introduced a simplified ValueRecord class for such dictionaries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Introduction of new functionality to the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants