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

[3.x] [Net] ENet poll optimizations, fragmented unreliable transfer. #53130

Merged
merged 2 commits into from
Dec 16, 2021

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Sep 27, 2021

In this PR:

  • ENet now sends fragmented packets unreliably too.
  • ENet poll now only service the connection once.

2 out of the 3 changes in #51466 (+ debug warning when sending unreliable fragmented packets).
The other change (sendmsg/WSASendTo) needs more testing because according to MS docs WSABUF is not available in UWP apps (though WSASendTo is?)

Bugsquad edit: 3.x version of #53129.

It used to always send them reliably when transfer mode was unreliable
or ordered if the packet size was more then the enet host MTU (1400
bytes by default).

This commit also adds a warning when debug is enabled to explain the
effects of sending fragmented packets unreliably.
It used to call `enet_host_service` until all events were consumed, but
that also meant constantly polling the connection leading to potentially
unbounded processing time.

It now only service the connection once, and instead consumes all the
retrieved events via `enet_host_check_events`.
@Faless Faless added this to the 3.4 milestone Sep 27, 2021
@Faless Faless requested a review from a team as a code owner September 27, 2021 13:36
@Faless Faless changed the title [Net] ENet poll optimizations, fragmented unreliable transfer. [3.x] [Net] ENet poll optimizations, fragmented unreliable transfer. Sep 27, 2021
@jordo
Copy link
Contributor

jordo commented Sep 30, 2021

I'd really like to see all of #51466 merged. We're depending on some of the efficiencies in sendMsg. WSASendTo is included in UWP, so anything in the header definition (LPWSABUF) would be included as well no? But agree it could use more testing, but I believe @mhilbrunner tested on UWP?

@mhilbrunner
Copy link
Member

Didn't have a chance to test yet, unfortunately.

@akien-mga
Copy link
Member

Does this need an update to match what got merged in #53129?

@Faless
Copy link
Collaborator Author

Faless commented Dec 16, 2021

@akien-mga no, this is up to date, and good to merge to 3.x (without further cherry pick since it changes an old time behaviour).

@akien-mga akien-mga merged commit 14d439d into godotengine:3.x Dec 16, 2021
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants