Skip to content

Draft: Speed improvements #659

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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

srfwx
Copy link
Contributor

@srfwx srfwx commented Dec 4, 2024

Fixes: #658 #625

Various refactoring allows for a substantial increase in performance of the Record initialization.

  • _parse_values was heavily refactored to avoid multiple similar checks and unnecessary processing
  • in turn, get_return now only needs to work on Record object and drops legacy code from Netbox 2.7
  • _endpoint_from_url can work with string instead of using the urlsplit library
  • finally caching of apps/endpoints avoid constant reinstantiation of similar objects

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

srfwx commented Dec 10, 2024

@arthanson the size of this PR may have gotten a little bit out of hand.
I assume you may want to break it into into smaller changes.

As a summary here is what's been implemented so far:

  • Cache of Endpoints on App
  • Cache nested Records on Endpoints
  • Adds class ValueRecord for dict that don't include an endpoint (contrary to class Record)
  • Improves speed of computation of endpoint from URL
  • Lazy computation of Record.endpoint
  • Improves value parsing performance and readability

Please let me know what you think and what you'd like to possibly see as individual PR.

@@ -109,3 +109,5 @@
"wireless.WirelessLANGroup": None,
"wireless.wirelesslink": None,
}

TYPE_CONTENT_MAPPER = {v: k for k, v in CONTENT_TYPE_MAPPER.items() if v is not None}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this for? I don't see it used anywhere?

@@ -151,6 +151,7 @@ def __str__(self):


class Interfaces(TraceableRecord):
device = Devices
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you put this in a separate PR, it doesn't look like a speed improvement but a bug fix (if a bug fix and there isn't an issue for it, please open an issue).

@@ -53,8 +84,12 @@ def __init__(self, api, app, name, model=None):
endpoint=self.name,
)
self._choices = None
self._init_cache()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is only called in one place and has a function defined for it - can you just put it inline with a comment instead, not sure why we need a separate function for this?

@@ -20,6 +20,37 @@
RESERVED_KWARGS = ()


class CachedRecordRegistry:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe? Split this and the one in core/app.py into two separate PR's? Although both are caching - the seem like functionally separate functionality that can be reviewed separately (and potentially backed out separately if any issues).

@jnovinger
Copy link
Member

jnovinger commented Jun 12, 2025

@srfwx , can you address/answer Arthur's concerns/questions? Otherwise, we'll need to close this out as stale work before too long.

Also, please address the conflict if it's not resolved by other changes.

@srfwx
Copy link
Contributor Author

srfwx commented Jun 12, 2025

Hi @jnovinger ,
Yes sure, I'll get to it as soon as possible.
Note that it took 5 months to get initial feedback so as you can imagine I've switched to other topics in the meantime... But I'll try to allocate time for this in the coming couple of weeks if that's alright

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.

Speed improvements on Record init
3 participants