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

Improve octoprint gcode analysis speed #2730

Merged
merged 5 commits into from
Jul 13, 2018

Conversation

eyal0
Copy link
Contributor

@eyal0 eyal0 commented Jul 13, 2018

  • 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, or maintenance if it's
    a bug fix for an issue present in 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?

Increase gcode analysis speed by around 30%

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

Run this command with and without the patch:

time python ./src/octoprint/cli/analysis.py --speed-x=6000 --speed-y=6000 --max-t=10 --throttle=0.0 --throttle-lines=100 BIG_GCODE_FILE.gco

You will need to patch in the two lines that were added to analysis.py so that you can run analysis.py. I recommended patching those in, too, because they do no harm and make it easier to profile the analysis. To profile, run:

python -m cProfile -s tottime ./src/octoprint/cli/analysis.py --speed-x=6000 --speed-y=6000 --max-t=10 --throttle=0.0 --throttle-lines=100 BIG_GCODE_FILE.gco | less

Notice that the speed is improved but the output is unaffected.

Any background context you want to provide?

While working on the analysis factory, I noticed that default analysis is very slow and tried to improve it. It would run faster if it were in c but that would be a lot more work. This is a good start.

@eyal0 eyal0 changed the base branch from master to maintenance July 13, 2018 04:57
@arhi
Copy link

arhi commented Jul 13, 2018

I'm blind or replacing if None with if float("inf") cuts 30% of the execution time? I'd report that as python bug :(

@eyal0
Copy link
Contributor Author

eyal0 commented Jul 13, 2018

Three changes.

  1. Don't import math on each call to getFloat
  2. Unroll the loop so that we don't need to call getattr, which is doing reflection. Also, loop unrolled so that's faster.
  3. We don't need to test against None each time by using infinity.

Check for yourself. I measured 30% on my laptop.

@eyal0
Copy link
Contributor Author

eyal0 commented Jul 13, 2018

I faked unit tests. The comparison should be: is min greater than max then return None.

@foosel
Copy link
Member

foosel commented Jul 13, 2018

Looks like you broke one of the existing tests there: travis report. The Examples in the doc strings are doc tests. Could you fix that?

Also: Thanks for this. I have to admit I didn't know one could do float("inf") and learnt something new today! 👍

@foosel foosel added the needs some work There are some things to do before this PR can be merged label Jul 13, 2018
eyal0 added 2 commits July 13, 2018 11:57
This reverts commit 792fe32.

Most of the speed-up comes from removing the unneeded import and the unrolling so no big deal.
@foosel foosel merged commit 9559f09 into OctoPrint:maintenance Jul 13, 2018
@eyal0
Copy link
Contributor Author

eyal0 commented Jul 13, 2018

I undid the last two commits with the infinity stuff. Those didn't affect the speed of the code as much as the rest. Lost just 5 out of the 30 percent speed-up.

Those unit tests are kind of broken anyway. They test the case where MinMax3D is called with a None coordinate but that does actually work? I don't think so:

	>>> none_after_found = MinMax3D()
	>>> none_after_found.record(Vector3D(1,1,1))
	>>> none_after_found.record(Vector3D(2,2,2))
	>>> none_after_found.record(Vector3D(2,None,2))
	>>> str(none_after_found.size)
	Vector3D(x=1, y=0.0, z=1, length=1.41421356237)

I would expect that the None means no data so size would be 1x1x1. Instead, the y size was set to zero. This is for the new and old code.

So really, this function doesn't work properly with None in any of x, y or z. So all the test cases where None is in one of the inputs isn't valid anyway. Luckily, the code never inputs a None so it doesn't break.

5% isn't that much but on RPi is can feel like a lot! If you want it done, let me know and I'll make another PR.

@foosel
Copy link
Member

foosel commented Jul 13, 2018

I agree, looking at those tests now I'm not entirely sure what's up there either. I'll happily accept another PR that gets rid of anything senseless there ;)

@foosel foosel added this to the 1.3.10 milestone Oct 31, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs some work There are some things to do before this PR can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants