iv-org / lsquic.cr

Crystal bindings to LiteSpeed QUIC
MIT License
22 stars 7 forks source link

Update lsquic binding lifecycle #7

Closed tenpura-shrimp closed 3 years ago

tenpura-shrimp commented 3 years ago

The previous code did not have an implementation to set timers for processing connections. This leads to an occasional deadlock where process_connections must be called to continue. This change implements this timer modelled after the lsquic reference implementation.

Reference: https://github.com/litespeedtech/lsquic/blob/master/bin/prog.c#L535

The existing code reads each message from the socket individually and calls engine_process_conns after each message. This seems inefficient as it leads to excess calls engine_process_conns.

This changes the code that reads from the socket to read all available messages before calling engine_process_conns based off of the logic in the reference implementation.

reference: https://github.com/litespeedtech/lsquic/blob/master/bin/test_common.c#L737

tenpura-shrimp commented 3 years ago

results from a test I ran of livestream manifest, measured from the browser

Latencies with the original code: Mean: 74ms lowlatencyorig fulllatencyoriginal

Latencies with the updated code: Mean: 67ms lowlatencynew fullatencynew

unixfox commented 3 years ago

Does this need testing now then?

Perflyst commented 3 years ago

Yes, would be good to test it first.

Perflyst commented 3 years ago

Patch is now in testing on my server

Perflyst commented 3 years ago

As far as I can see @tenpura-shrimp force pushed the two commits after @saltycrys reviewed. Could you take a look again @saltycrys? Afterwards we can merge and release 2.23.1