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

Replace multiple globs with single regex #4221

Merged
merged 2 commits into from Sep 9, 2021
Merged

Replace multiple globs with single regex #4221

merged 2 commits into from Sep 9, 2021

Conversation

flaviut
Copy link
Contributor

@flaviut flaviut commented Sep 6, 2021

  • 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?

The existing code lists the directory contents 7 times. This usually
isn't a big deal, but I'm running Octoprint on a really slow ARM board,
so this ends up taking 123ms (coming out to 17ms per call).

We can speed this up a lot by only listing the files once, and searching
through them. With this change, this function is fast enough that it
doesn't show up in the profiler

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

This has been tested on v1.6.1.

Any background context you want to provide?

What are the relevant tickets if any?

Screenshots (if appropriate)

Further notes

@github-actions github-actions bot added the approved Issue has been approved by the bot or manually for further processing label Sep 6, 2021
flaviut and others added 2 commits September 9, 2021 15:37
The existing code lists the directory contents 7 times. This usually
isn't a big deal, but I'm running Octoprint on a really slow ARM board,
so this ends up taking 123ms (coming out to 17ms per call).

We can speed this up a lot by only listing the files once, and searching
through them. With this change, this function is fast enough that it
doesn't show up in the profiler
We only use the search in one single place, it's not
part of an external API and three lines only. It makes
sense to internalize it into serialList.

Also, python 2 doesn't yet have os.scandir, so use the
fallback implementation in that case.
@flaviut
Copy link
Contributor Author

flaviut commented Sep 9, 2021

Thanks for catching the py2 issue. I can't wait until I can forget about it entirely.

I disagree with inlining the function. It feels like a violation of the single-responsibility principle. But it's very subjective, and this is your code :)

@foosel
Copy link
Member

foosel commented Sep 9, 2021

Well, we have two choices here, outlining the windows specific stuff, or inlining the *nix specific stuff. The latter feels more in line with other locations in the code. Alternatively, one could argue that there might be a need for such a function among the util classes, but I'm unsure if the need for that really exists. Should this ever change it can still be changed with a quick refactoring I guess.

@foosel foosel merged commit 01855da into OctoPrint:maintenance Sep 9, 2021
@flaviut flaviut deleted the reduce-glob branch September 9, 2021 15:10
@foosel foosel added this to the 1.8.0 milestone Sep 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Issue has been approved by the bot or manually for further processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants