pygame-ce
pygame-ce copied to clipboard
pygame-community
Performance improvement for draw functions
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
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 ## macro syntax to conditionally select functions so whole paths are specific to bit depth.
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.
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
| Function tested | Newest release | My branch |
|---|---|---|
| Draw Small Line | 0.735 | 0.596 |
| Draw Big Line | 0.803 | 0.450 |
| Draw Small Arc | 3.181 | 2.487 |
| Draw Big Arc | 9.893 | 7.395 |
| Draw Small Thick Circle | 4.125 | 3.820 |
| Draw Big Thick Circle | 14.773 | 14.331 |
| Draw Small Circle 1 width | 1.362 | 1.159 |
| Draw Big Circle 1 width | 2.485 | 1.440 |
| Draw Small Filled Circle | 1.222 | 1.220 |
| Draw Big Filled Circle | 14.795 | 13.881 |
| Draw Small Ellipse | 1.368 | 1.219 |
| Draw Big Ellipse | 9.893 | 9.354 |
| Draw Small Filled Ellipse | 0.613 | 0.588 |
| Draw Big Filled Ellipse | 9.365 | 9.205 |
| Draw Small Thick Rect | 0.835 | 0.818 |
| Draw Big Thick Rect | 8.064 | 8.075 |
| Draw Small Filled Triangle | 0.242 | 0.219 |
| Draw Big Filled Triangle | 18.950 | 18.270 |
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.
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 .
