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

Etag and refresh optimizations #3346

Closed

Conversation

justfalter
Copy link
Contributor

@justfalter justfalter commented Nov 25, 2019

  • Your changes are not possible to do through a plugin and relevant
    to a large audience (ideally all users of OctoPrint)
  • If your changes are large or otherwise disruptive: You have
    made sure your changes don't interfere with current development by
    talking it through with the maintainers, e.g. through a
    Brainstorming ticket
  • Your PR targets OctoPrint's devel branch if it's a completely
    new feature, or maintenance if it's a bug fix or improvement of
    existing functionality for the current stable version (no PRs
    against master or anything else please)
  • Your PR was opened from a custom branch on your repository
    (no PRs from your version of master, maintenance or devel please),
    e.g. dev/my_new_feature or fix/my_bugfix
  • Your PR only contains relevant changes: no unrelated files,
    no dead code, ideally only one commit - rebase and squash your PR
    if necessary!
  • Your changes follow the existing coding style
  • If your changes include style sheets: You have modified the
    .less source files, not the .css files (those are generated with
    lessc)
  • You have tested your changes (please state how!) - ideally you
    have added unit tests
  • You have run the existing unit tests against your changes and
    nothing broke
  • You have added yourself to the AUTHORS.md file :)

What does this PR do and why is it necessary?

Two performance improvements that came as a result of profiling OctoPrint. My primary goal has been to reduce the CPU impact when it comes to serving /. All performance numbers I'm reporting here are from running using python 3.7.5 on my 2016 Macbook Pro. I expect the improvements to be even more significant on a typical raspberry pi.

1. Improve performance of etag calculation for / (root route):

  • Only compute etag once (rather than twice) per request.
  • Only stat files once per request.
    • Avoid calling isdir or isfile in collect_files.
    • Leave it to compute_lastmodified to stat files, which tells us if the path exists, if it is a file, and it's last modified.
  • Ultimately, this eliminates ~70% of stat system calls. This has a bigger impact on raspberry PI systems.

Timing (as reported by Tornado) on my 2016 Macbook Pro when fetching / (curl -o /dev/null http://octoprint/):

  • Before: ~50ms
  • After: ~38ms

2. Memoize settings hashes to speed up cache refresh:
Memoize settings.config_hash and settings.effective_hash. The
hashes will only be calculated on the first call to the respective
functions. These memoized values are invalidated when the settings
object becomes dirty, or the settings are loaded from the config file.

Reason for change: During a refresh of the root route (/), the hash of
settings.effective.yaml is calculated 5 times. Each call lead to a
large configuration object to be serialized to a YAML string, which would
then be hashed. As it turns out, serializing to YAML is expensive.

Note: while I do not see settings.config_hash hit during a refresh,
but making a change to it was easy and appropriate. It's possible that
other areas of the application may see some benefit.

Performance (on my 2016 Macbook Pro:

  • building of initial cache on startup:
    • before: ~3.4s
    • after: ~2.9s
  • loading http://octoprint/?_refresh=1
    • before: ~1100ms
    • after: ~200ms

How was it tested? How can it be tested by the reviewer?

Manually.
I ran pytest, and everything passes.

Any background context you want to provide?

What are the relevant tickets if any?

Screenshots (if appropriate)

Further notes

- Only compute etag once (rather than twice) per request.
- Only stat files once per request.
  - Avoid calling `isdir` or `isfile` in `collect_files`.
  - Leave it to `compute_lastmodified` to stat files, which tells us if
  the path exists, if it is a file, and it's last modified.
  - Reduces the number of stat systemcalls by 50%.
Memoize `settings.config_hash` and `settings.effective_hash`. The
hashes will only be calculated on the first call to the respective
functions. These memoized values are invalidated when the settings
object becomes dirty, or the settings are loaded from the config file.

Reason for change: During a refresh of the root route (/), the hash of
`settings.effective.yaml` is calculated 5 times. Each call lead to a
large configuration object to be serialized to a YAML string, which would
then be hashed. As it turns out, serializing to YAML is expensive.

Note: while I do not see `settings.config_hash` hit during a refresh,
but making a change to it was easy and appropriate. It's possible that
other areas of the application may see some benefit.

Performance (on my 2016 Macbook Pro:
- building of initial cache on startup:
  - before: ~3.4s
  - after:  ~2.9s
- loading `http://octoprint/?_refresh=1`
  - before: ~1100ms
  - after:  ~200ms
@foosel foosel added this to the 1.4.1 milestone Nov 25, 2019
@foosel
Copy link
Member

foosel commented Nov 25, 2019

Thanks. I've scheduled that for 1.4.1 to give it a more thorough look than I can currently do due to 1.4.0 stress.

@foosel
Copy link
Member

foosel commented Apr 6, 2020

Manually merged into maintenance (realized too late that this was targeting devel and should have been rebased first)

@foosel foosel closed this Apr 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants