-
-
Notifications
You must be signed in to change notification settings - Fork 168
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
Performance improvement for draw functions #2551
base: main
Are you sure you want to change the base?
Conversation
Hey MightyJosip! I'm just starting to look at this and it's a lot of lines changed, I think it would be helpful if you explained the rationale behind what you've done. I see you've linked #2517 but it seems like what you've done here is more complex than what MyreMylar pointed out. You say that functions "should" see a speedup, have you done any performance testing? |
Ok I'm starting to see the approach now, you've used heavy use of This is not a strategy I've used before. In the blitter code my strategy was to pass code as a parameter argument into macros. Before this PR, the draw module is 52.5kb, afterwards it is 86.5 kb. |
Ok I might be up to speed now. Myre was talking about function pointers in heavy loops on discord (that must've been your previous strategy), so now you've done it so that the code path is duplicate-compiled farther up. Now I look at it and one of my first impressions is that it's excessively duplicated. Maybe this is what we get to eventually or maybe there is a better way? I'm not sure. |
Yeah an actual reproducible program and results would make things easier. |
Based on the @MyreMylar test in the issue: https://gist.github.com/MightyJosip/dca7901bb48f053bdb10e0f23244afa1 (file test.py) Tested on current release (2.3.2) and my branch. TLDR of the results
|
This optimization effort is truly impressive and worth celebrating. However, the number of changes and the effort needed to review them make it quite challenging. Since these are mostly macro compositions, it's hard to follow and review them properly. I'm also worried that future improvements will be difficult. Could we split this into multiple pull requests? For example, one for lines, one for circles, and so on. This would make it easier and faster to review, test, read, and suggest improvements. Let me know what you think. |
Hello @itzpr3d4t0r , I was also thinking about the same thing when I was reading the changes. I'm also seeing 2 PR (this one and #2080) with the same objective. IMO we should open an issue about general draw performance improvements, and link these 2 PRs to the issue, so if anyone else want to work on it, he can see the work of joking and temmie. Let me know what you think @MightyJosip and @Temmie3754 . |
Fix #2517
This is kinda big refactoring of draw module, function changed are single pixel line thickness, arc, circle, ellipse, triangle, and multi pixel thick rectangle. Most of those functions should see solid performance boost (depends on the function). I didn't change circle quadrant (and corresponding round rect), it needs big refactoring, and right now I don't want that to be roadblocker to this. Also I don't think I could accomplish much with thick line