Skip to content

Avoid unnecessary PlatformMessage copies in plugin callback loop #25

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

Merged
merged 3 commits into from
Nov 4, 2018

Conversation

pas2k
Copy link
Contributor

@pas2k pas2k commented Nov 4, 2018

Reworked PlatformMessage propagation to use pointers.

All golang values are passed by value. That means that when you pass a struct to a function, that's about as efficient as passing all of the struct members as parameters, including copying all the
nested strings.

This might be okay, or even beneficial from GC perspective for small structs such as PointerEvent, but is a really bad idea when used in loops (such as callback loop made by the plugin system).

The only reason you don't see much of pointers in golang stdlib is, it primarily operates on interfaces, while the interfaces themselves are mostly implemented on pointer, not value types.

Also reworked access to PlatformMessage.Message into pointers, for the same reason. No need to change the member itself into pointer -- it's only meaningful to pass with the corresponding PlatformMessage.

@pchampio pchampio merged commit f13998e into go-flutter-desktop:master Nov 4, 2018
@pchampio
Copy link
Member

pchampio commented Nov 4, 2018

@pas2k,
You pointed out a lot of mistake/slip made.
The work, comments and PR you made over the last week were always top notch.
I am giving you commit access to the repo 🥇

@pas2k
Copy link
Contributor Author

pas2k commented Nov 4, 2018

@Drakirus, thanks! I'll try to do my best!

@pas2k pas2k deleted the pmessage_pointer branch November 4, 2018 22:39
@pchampio pchampio mentioned this pull request Nov 4, 2018
@winner484 winner484 mentioned this pull request Mar 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants