Window: Revert swap forced sync (#4219) as it causes performance issue #6578
Conversation
|
Thanks for opening your first pull request here! 💖 Please check out our contributing guidelines. |
kivy/uix/textinput.py
Outdated
| @@ -2252,6 +2252,13 @@ def _get_line_options(self): | |||
|
|
|||
| def _create_line_label(self, text, hint=False): | |||
| # Create a label from a text, using line options | |||
|
|
|||
There was a problem hiding this comment.
what this have to do with the actual issue?
There was a problem hiding this comment.
Oops! the last two commits are not related to issue. My mistake!
There was a problem hiding this comment.
Then could you remove this commit if it's not related. Also try to squash your changes so it's only one commit.
|
@quitegreensky how much of a regression? Can you add some data? |
The whole story began from #2002. There are all informations here. |
There was a problem hiding this comment.
Thanks for looking into it and giving a try.
FYI there was an attempt to revert it in #6242
Let's see what comes up this time
kivy/uix/textinput.py
Outdated
| @@ -2252,6 +2252,13 @@ def _get_line_options(self): | |||
|
|
|||
| def _create_line_label(self, text, hint=False): | |||
| # Create a label from a text, using line options | |||
|
|
|||
There was a problem hiding this comment.
Then could you remove this commit if it's not related. Also try to squash your changes so it's only one commit.
| with nogil: | ||
| SDL_GL_SwapWindow(win) | ||
| cgl.glFinish() | ||
| SDL_GL_SwapWindow(self.win) |
There was a problem hiding this comment.
I'm actually curious to know which change/line specifically made the regression and why
|
I'm worried about merging this, given that we didn't merge the last request to revert for the reasons stated there. We could maybe disable it on mobile, but that feels hacky. Either way, it would be good to get confirmation that specifically reverting this commit fixes the fps issues. |
|
It seems the original fix #4219 was for a specific situation that caused a blocking of the main app when using multiple background threads, and getting a gl block due to swap being deferred by default by the drivers. Calling glFinish() ensured the swap on previous line happened immediately, however this also leads to a general slowdown as mentioned by a few people #6242 and in current thread. @matham @tito how about this be made a compile time option? For people that know what they are doing they could enable this, we could add information about it in the doc. |
|
If we can confirm that disabling glfinish fixes the issue, then unless someone has better ideas why having glfinish would lead to a unnecessary slowdown, I think a compile, or runtime option is the only solution I can think of. It's not great, hence why I thought it was hacky. Because when someone new to kivy runs into this issue, since it'll use glfinish by default, they probably won't know to lookup this issue and workaround. And they'd just assume it's a kivy problem. Perhaps disabling glfinish by default is a better option? And have a config option to turn it on? |
Update sdl2.pxi
I would assume that this makes much more sense. However Let's add some real Is this issue only on android? Does it effect iOS? Desktops? We probably should have these tests in kivy test suit even if they might not run every time? |
|
I'm strongly in favour of reverting this, it looks to me like we're taking a huge performance hit to improve a relatively rare use case. We can still improve it later. Edit: Also, this is really a big deal on Android. As already linked, kivy/python-for-android#2002 has an excellent simple test application demonstrating the performance drops. If we don't merge this to Kivy fairly soon, I'm thinking to have p4a patch it out, it's that bad. |
|
Thanks for your feedback on this @inclement |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
Ok, I merged it. I guess if someone wants to implement a config option that optionally enables the original patch they are welcome. |
Kivy 4219 revert was applied upstream via: kivy/kivy#6578 eth-abi, eth-account and web3 recipes are no longer required as we found a way to make F-Droid build with Python 3.6. gevent recipe got fixed p4a upstream.
Kivy 4219 revert was applied upstream via: kivy/kivy#6578 eth-abi, eth-account and web3 recipes are no longer required as we found a way to make F-Droid build with Python 3.6. gevent recipe got fixed p4a upstream.
matham
changed the title
#4219
These commits actually cause fps regression. I found this by doing some version comparisons. looking at getting this revert will solve the issue.