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

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

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

gwen-lg
Copy link
Contributor

@gwen-lg gwen-lg commented Aug 16, 2022

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 :
image
image

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

emilk commented Aug 25, 2022

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
Copy link
Contributor Author

gwen-lg commented Aug 25, 2022

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 gwen-lg force-pushed the async_client_connecting_local branch from 5da34cb to 94d0607 Compare September 29, 2022 20:43
@gwen-lg
Copy link
Contributor Author

gwen-lg commented Sep 29, 2022

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
Copy link
Member

TimonPost commented Oct 10, 2022

@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 TimonPost assigned TimonPost and unassigned VZout Oct 10, 2022
# Architecture
## server
Listens for incoming connections and streams them puffin profiler data.
- struct Server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

[dependencies]
anyhow = "1.0"
crossbeam-channel = "0.5"
async-executor = "1.4.1"
Copy link
Member

@TimonPost TimonPost Oct 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@TimonPost TimonPost Dec 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

crossbeam-channel = "0.5"
async-executor = "1.4.1"
async-std = "1.12"
flume = "0.10.14"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flume seems like a nice change for crossbeams_channel

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

plot

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
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
@gwen-lg gwen-lg force-pushed the async_client_connecting_local branch from 94d0607 to ae55b4b Compare November 24, 2022 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants