Request for Comments: puffin_http: use async local task #89

gwen-lg · 2022-08-16T12:50:13Z

Checklist

I have read the Contributor Guide
I have read and agree to the Code of Conduct
I have added a description of my changes and why I'd like them included in the section below

Description of Changes

Use async Rust to manage puffin server (puffin_http) tasks. An regroup it in one dedicated thread.
I think it's proper

Changes are not totally clean, but before improve it, I like to know if your are interested by this approach.

I have tested this change with my dedicated app : https://github.com/gwen-lg/puffin-test-app, but I think it's need test with real application/running game.

Also I have identified this think to do before merge it :

tried to have task name management
clean commented code
remove comment of crossbeam_channel
finish rewrite of architecture section of puffin_http/readme.md

Related Issues

This change are not related to a github issue.
But this change permit to implement

A captures of result :

emilk · 2022-08-25T06:15:46Z

This adds A LOT of dependencies, which will greatly increase compile times for any user of puffin_http.

What concrete benefits does this PR bring to justify that?

gwen-lg · 2022-08-25T10:48:52Z

Sorry, I have forgotten this part in pull request.

I have stared this after have tried to use puffin with app without frame (like a cooking process).
With some test, I wasn't able to get first frame (loading) of an application and no more have a capture for and application without frame (simulated with a one frame application).
This change allow :

to a client to connect without the end of the first frame.
to get the first frames, event when they are short.
add a functionality to wait connection of a client before continue app running (ex: wait_client_in_server )
get markers for "one run application" (application without frame)

PS: I have tested this with this : puffin-test-app

gwen-lg · 2022-09-29T20:53:08Z

I was thinking of the possibility of enable async with feature option.
This would be ideal for users, if there already have async code or if they want, they could enable async management.
But it's implies a lot of code duplicate.

What did you think of it?

TimonPost · 2022-10-10T20:09:17Z

@gwen-lg thanks for submitting the PR, sorry for the lack of activity, I will get back to the questions in this PR. I can try it for our internal project and see how it behaves and let you know if there is interest in continuing with the PR.

TimonPost · 2022-10-17T08:00:41Z

puffin_http/README.md

+# Architecture
+## server
+Listens for incoming connections and streams them puffin profiler data.
+- struct Server


Lets keep architectural docs in rust docs, no need to manually and separately document code

TimonPost · 2022-10-17T08:01:38Z

puffin_http/Cargo.toml

@@ -14,7 +14,10 @@ include = ["**/*.rs", "Cargo.toml", "README.md"]

 [dependencies]
 anyhow = "1.0"
-crossbeam-channel = "0.5"
+async-executor = "1.4.1"


Like you suggested, including an async run time into the library by default is something that we would like to prevent. Especially our users, among our selfs, might be using different runtimes already making them statically depended on an other one.

Maybe a new crate can be created that has this specific runtime which then executes the puffin_http

Migrate to async involves the use of async, await keyword in multiple place.
I don't think it's possible to create a new crate that use puffin_http as base and add "async" on top.

I think the best option to keep both, is use feature management of crates with some code duplication.
Are you interested in having me tried this option ?

PS: I haven't tested, but think than directly use of async-executor, instead of async-std task keep runtime local to crate, and avoid conflict if application use other async runtime.

Would be nice if we can just have async functions and let this whole discussion up to the user. Would be nice to see if we can explore having this indeed as a feature toggle. Such that you can optionally opt into the async api with just some duplication in library entry functions.

And so long as possible do not include any runtimes. Doesn't seem necessary, or do you think otherwise?

The interest to use async-executor directly is to keep all the puffin server work in a dedicated thread, "isolated" of the rest of the app. So I think having a runtimes (async-executor) included is important. And as it's related to a LocalExecutor only use in puffin_http/server.rs / in puffin-server thread, I supposed it's could work with app than use other runtime (like Tokio) without interference.
Possibly it can be deactivated with option for user than don't want an additional thread and prefer than puffin_server work run on the same executor than the rest of the app.
I think it better to target by default the user who don't have async and just want add profiling simply and working "out of the box".
PS: I'm not sure than puffin profiler is adapted for full async app. So not sure this is the type of user to target with this.

TimonPost · 2022-10-17T08:28:47Z

puffin_http/Cargo.toml

-crossbeam-channel = "0.5"
+async-executor = "1.4.1"
+async-std = "1.12"
+flume = "0.10.14"


flume seems like a nice change for crossbeams_channel

Are you interested to change from crossbeams_channel to flume even without async ?

I gave flume a go but the crossbeam benchmarks seems to indicate that crossbeam performed better than flume:

And also their own benchmarks seemed to show that. So I am not very sure if its desirable. Maybe I am missing something tho

Ok.
From what I have seen on flume crate page, most of the time crossbeam-channel is faster, but sometime flume do best.
It could be depend of principal usage in Puffin and whether the difference in performance is significant.
I have used flume in first place because crossbeam-channel don't manage async, and flume is presented as better than std (async ?) channel

Yea makes sense to opt for that solution in this case. So its good to me using flume i.e. of crossbeam. Unless someone has an better idea lets opt for that.

use Arc RwLock container to share clients list

this reflect change done when split management

used for client connection and frame send first introduction, not used ideally

ps-connect : to manage client connection ps-send : to manage frame send

from async-std instead of from std

this remove use of async function in retain. Async function don't work in retain with local executor.

print in log::info when packet channel receiver return an error

- "accept_client" - send_to_client - "write frame to client"

this avoid to spawn multiple threads for management of async task of puffin server. Now all puffin server task are executed in the same thread, a dedecated thread.

flume have better perf ?

Allowed because all task run in an unique Thread

Use Rc & RefCell instead of Arc & RwLock. With LocalExecutor, no need of Sync & Send trait

Use Rc instead of Arc. With LocalExecutor, no need of Sync & Send trait

repi assigned VZout Aug 24, 2022

gwen-lg force-pushed the async_client_connecting_local branch from edf9f6b to 5da34cb Compare August 24, 2022 21:28

gwen-lg force-pushed the async_client_connecting_local branch from 5da34cb to 94d0607 Compare September 29, 2022 20:43

TimonPost assigned TimonPost and unassigned VZout Oct 10, 2022

TimonPost requested changes Oct 17, 2022

View reviewed changes

gwen-lg added 20 commits November 20, 2022 20:59

Add Architecture section in puffin_http Readme

5cf6477

split client connection and frame send management

2f8e96a

use Arc RwLock container to share clients list

rename 'puffin server impl' into 'puffin server send'

00e23a1

this reflect change done when split management

add async with async-std

d4c5dad

used for client connection and frame send first introduction, not used ideally

split puffin-server thread into tasks

be61dd1

ps-connect : to manage client connection ps-send : to manage frame send

use task instead of thread for Client

6a2f2d5

add run into Server and use thread to process it

ce6bd07

use async-std channel to replace crossbeam_channel

014e6ca

use async Arc and RwLock instead of sync

bb481e6

from async-std instead of from std

use net::SocketAddr from async-std instead of std

31c0146

move tcp_listener creation into async task ps-connect

c00ffa6

remove use of retain in PuffinServerSend::send

c015d39

this remove use of async function in retain. Async function don't work in retain with local executor.

Add log for error in client loop

740c7bd

print in log::info when packet channel receiver return an error

add some profile scope and function markers

b9474bb

- "accept_client" - send_to_client - "write frame to client"

use dedicated LocalExecutor for puffin_http server

8bc1a04

this avoid to spawn multiple threads for management of async task of puffin server. Now all puffin server task are executed in the same thread, a dedecated thread.

replace channel from async-std to flume

8865f2f

flume have better perf ?

wip: update Architecture section in puffin_http README.md

8979bc3

Clean: change unneeded Arc into simple RC

17bc2c5

Allowed because all task run in an unique Thread

remove useless atomic to client variable

0c52886

Use Rc & RefCell instead of Arc & RwLock. With LocalExecutor, no need of Sync & Send trait

remove useless atomic to executor variable

ae55b4b

Use Rc instead of Arc. With LocalExecutor, no need of Sync & Send trait

gwen-lg force-pushed the async_client_connecting_local branch from 94d0607 to ae55b4b Compare November 24, 2022 21:39

dont borrow_mut cliens during send_to_client calls

7d6c5f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request for Comments: puffin_http: use async local task #89

Request for Comments: puffin_http: use async local task #89

gwen-lg commented Aug 16, 2022 •

edited

Loading

emilk commented Aug 25, 2022

gwen-lg commented Aug 25, 2022

gwen-lg commented Sep 29, 2022

TimonPost commented Oct 10, 2022 •

edited

Loading

TimonPost Oct 17, 2022

TimonPost Oct 17, 2022 •

edited

Loading

TimonPost Oct 17, 2022

gwen-lg Oct 26, 2022

TimonPost Dec 11, 2022 •

edited

Loading

TimonPost Dec 11, 2022

gwen-lg Dec 15, 2022

TimonPost Oct 17, 2022

gwen-lg Oct 26, 2022

TimonPost Oct 26, 2022

gwen-lg Oct 26, 2022

TimonPost Dec 11, 2022

Request for Comments: puffin_http: use async local task #89

Are you sure you want to change the base?

Request for Comments: puffin_http: use async local task #89

Conversation

gwen-lg commented Aug 16, 2022 • edited Loading

Checklist

Description of Changes

Related Issues

emilk commented Aug 25, 2022

gwen-lg commented Aug 25, 2022

gwen-lg commented Sep 29, 2022

TimonPost commented Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

TimonPost Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimonPost Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwen-lg commented Aug 16, 2022 •

edited

Loading

TimonPost commented Oct 10, 2022 •

edited

Loading

TimonPost Oct 17, 2022 •

edited

Loading

TimonPost Dec 11, 2022 •

edited

Loading