Conversion of MessageBody to accept Pin in poll_next by dunnock · Pull Request #1332 · actix/actix-web

dunnock · 2020-01-29T11:36:28Z

Starting conversion of MessageBody to accept poll_next(self: Pin<&mut Self...) as it was adviced in #1321 (comment) .

This is breaking change and probably should be included in the next version, amended versions accordingly.

Following versions updated as part of this PR to align with semver:

actix-http 1 -> 2.0.0-alpha

Checklist:

Move to another PR / tasks

Publish actix-http with version 2.0.0-alpha to crates
Fix dependencies (actix-web and awc), update versions:
actix-web 2.0 -> 2.1 (probably should go to 3.0?)
awc 1 -> 2

JohnTitor · 2020-01-30T14:09:30Z

Could you reword b55aa2e? Otherwise it'll close mentioned issue.

- Convert MessageBody to accept Pin in poll_next - add CHANGES and increase versions aligned to semver - update crates to accomodate MessageBody Pin change - fix tests and dependencies

dunnock · 2020-01-30T15:39:23Z

Could you reword b55aa2e? Otherwise it'll close mentioned issue.

Done, had to squash them but it seems no conflicts so ok

dunnock · 2020-01-30T15:53:35Z

After running benchmarks and separately load testing actix server with ab I do not see any significant difference in performance, pinned version seems working slightly faster but most likely it is just random variance:

ApacheBenchmark

Tests performed on ubuntu with 32GB RAM and 40 CPU

> ab -n 1000000 -c 40 -k ...

Version from this branch using Pin in poll_next():

> ab -n 1000000 -c 40 -k ...
Concurrency Level:      40
Time taken for tests:   29.673 seconds
Complete requests:      1000000
Failed requests:        0
Keep-Alive requests:    1000000
Total transferred:      220000000 bytes
HTML transferred:       119000000 bytes
Requests per second:    33701.00 [#/sec] (mean)
Time per request:       1.187 [ms] (mean)
Time per request:       0.030 [ms] (mean, across all concurrent requests)
Transfer rate:          7240.45 [Kbytes/sec] received

Version from master branch:

Concurrency Level:      40
Time taken for tests:   30.945 seconds
Complete requests:      1000000
Failed requests:        0
Keep-Alive requests:    1000000
Total transferred:      220000000 bytes
HTML transferred:       119000000 bytes
Requests per second:    32315.58 [#/sec] (mean)
Time per request:       1.238 [ms] (mean)
Time per request:       0.031 [ms] (mean, across all concurrent requests)
Transfer rate:          6942.80 [Kbytes/sec] received

cargo bench

Version with Pin:

get_body_async_burst    time:   [118.97 us 120.46 us 121.87 us]

Version from Master:

get_body_async_burst    time:   [121.38 us 122.17 us 122.90 us]

JohnTitor · 2020-01-30T15:57:37Z

Rough reviews at this point:

Do not update actix-{identify, session} here, we're moving out from here now, see https://github.com/actix/actix-extras.
I'm negative to update all workspace members at once, we should do step by step as possible.
The version numbers should be alpha since we may add breaking changes later. And I'm also negative to bump them here.

At a glance, the benchmark you commented looks good to me, so the change should be fine roughly.

dunnock · 2020-01-30T17:31:07Z

@JohnTitor need your advice. I initially wanted to include only changes in actix-http and point rest of crates to version 1.0.1. Though it appeared problematic as there is dependency chain:
actix-http dev dependency is actix-http-test { ./test-server } which depends on awc, actix-web and actix-http
In turn awc depends on actix-http, actix-web and actix-http-test
actix-web depends on actix-http too

To illustrate the issue I have just pushed commit pointing actix-http to actix-http-test:1.0.0:

Problem is actix-http-test crate is used only to run some tests within actix-http crate, though if I do not provide updated version of awc and actix-web actix-http tests do not compile as they use different versions of structs and traits : actix-http:1.0.1 and actix-http:2.0.0.

Once I update actix-web, crates actix-{identify, session} fail to compile.

One possible solution is to move out actix-http tests which depend on actix-http-test crate. Another option would be to comment out those tests temporarily.

JohnTitor · 2020-01-31T01:26:38Z

One safe way is to prepare another branch and work there. You can ignore workspace members that don't compile there. I just prepared msg-body branch.
And we can ignore actix-{identity, session} once the alpha version is released and they're published on extras repo.

dunnock · 2020-01-31T12:40:01Z

This is ready for review now with all 3 changes as requested.
I believe CI issues not related to this change.

Cargo.toml

cdbattags

Small things but otherwise looks great

actix-http/src/encoding/encoder.rs

actix-http/src/h1/dispatcher.rs

robjtede

Been thinkng about this for a couple days. It seems uneccesary to have MessageBody take the Unpin supertrait. You're handling the self Pin signatures correctly in most places anyway but it'll only takes a couple of modifications to make it work without the supertrait.

Eg. the Box and Box impls can use map_unchecked_mut calls. The h1 dispatcher maybe slightly more involved (just from changing a couple things) but given the h2 dispatcher is already using pin_project it seems achievable.

dunnock · 2020-02-03T21:03:20Z

@robjtede I have unlinked MessageBody from Unpin as you suggested. h1::dispatcher appeared quite complicated and requires strong review. DispatcherState and SendResponse required some unsafe code too.

Aaron1011 · 2020-02-08T19:22:10Z

@dunnock: I'm working on changes to pin-project that will help with the line that you mentioned. I can open a PR to address it once this is merged.

dunnock · 2020-02-09T12:18:18Z

@Aaron1011: If you mean taiki-e/pin-project#181 not sure how that will help here? FramedParts takes ownership of a stream, hence we are taking the Dispatcher::inner value but it is a Pin.

Aaron1011 · 2020-02-09T20:44:31Z

@dunnock: This is different - this idea is to have a pinned_replace method that will allow you to call mem::replace on a pinned enum, if the enum is currently set to a variant that has no #[pin] annotations.

I haven't yet opened a PR for it, but I will soon.

dunnock · 2020-02-09T21:27:54Z

@Aaron1011 in our case current enum variant is Pinned (DispatcherState::Normal), though we can confidently say that it's Pin not going to be called or accessed from anywhere as previous projections lifetime has ended on a line before.

dunnock · 2020-02-10T13:53:54Z

Last commits getting rid of unsafe code in a dispatcher with Aaron's idea to transfer not pinned fields (DispatchInner::io, *_buf, codec). Note that unwrap() should behave like a previous implementation which had explicit panic! under same unreachable conditions. @cetra3 @robjtede I guess it now addresses both concerns and can be reviewed again.

Compared to current master this PR removes 4 unsafe blocks from dispatcher not adding any new.

By benchmark it is still have no noticeable difference to the version in master:

> ab -c 40 -n 1000000 -k http://127.0.0.1:59993/
Concurrency Level:      40
Time taken for tests:   30.182 seconds
Complete requests:      1000000
Failed requests:        0
Keep-Alive requests:    1000000
Total transferred:      220000000 bytes
HTML transferred:       119000000 bytes
Requests per second:    33132.75 [#/sec] (mean)
Time per request:       1.207 [ms] (mean)
Time per request:       0.030 [ms] (mean, across all concurrent requests)
Transfer rate:          7118.36 [Kbytes/sec] received

robjtede · 2020-02-10T14:34:03Z

I'm liking the look of these changes @dunnock, thanks for putting the time in to this. Will give it a proper look over later today and resubmit review.

robjtede · 2020-02-10T22:26:57Z

Really good work; the tendrils of that trait ran much deeper than I thought. I'm glad we kept the Unpin only to necessary parts.

Use Actions fully

cdbattags · 2020-02-13T16:49:47Z

@dunnock amazing work on this

thanks for the time

Disable coverage for PRs

Dowwie · 2020-02-14T12:24:50Z

@JohnTitor do you see any reason not to merge? would rather you took the lead here..

JohnTitor

Really great as a first step! I left a comment for a nit point.

actix-http/Cargo.toml

…stream-1321

JohnTitor

Sorry for the delay! Let's go ahead :)

add test crashing with segfault according to actix#1321

e50ed55

dunnock mentioned this pull request Jan 29, 2020

Use Pin<Box<S>> in BodyStream and SizedStream #1328

Merged

dunnock force-pushed the bodystream-1321 branch from e2e3915 to cde68b5 Compare January 30, 2020 15:23

This is a squashed commit:

20a18fb

- Convert MessageBody to accept Pin in poll_next - add CHANGES and increase versions aligned to semver - update crates to accomodate MessageBody Pin change - fix tests and dependencies

dunnock force-pushed the bodystream-1321 branch from 47a0082 to 20a18fb Compare January 30, 2020 15:32

Merge branch 'master' into bodystream-1321

650caac

dunnock marked this pull request as ready for review January 30, 2020 15:54

Rollback actix-http-test dependency to show the issue

30959cb

dunnock changed the base branch from master to msg-body January 31, 2020 08:04

dunnock added 5 commits January 31, 2020 10:10

merge with msg-body

c3fce1c

rollback changes to actix-web, awc and test-server for now

e548e0f

disable weird poll test until actix-web based on actix-http:2

2271b11

rollback missed dependencies and CHANGES in crates except actix-http

82c7703

change actix-http version to alpha

c50a574

robjtede requested a review from a team January 31, 2020 13:45

cdbattags reviewed Jan 31, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

cdbattags reviewed Jan 31, 2020

View reviewed changes

actix-http/src/encoding/encoder.rs Show resolved Hide resolved

actix-http/src/h1/dispatcher.rs Outdated Show resolved Hide resolved

remove extra whitespaces and Unpins

99b72a6

cdbattags approved these changes Jan 31, 2020

View reviewed changes

dunnock changed the title ~~WIP: conversion of MessageBody to accept Pin in poll_next~~ Conversion of MessageBody to accept Pin in poll_next Feb 1, 2020

robjtede requested changes Feb 3, 2020

View reviewed changes

unlink MessageBody from Unpin

4135f16

dunnock added 4 commits February 10, 2020 12:12

Get rid of one more unsafe

d32f038

refactor dispatcher to avoid possible UB with DispatcherState Pin

8742ae9

Add Unpin to Body to get rid of unsafe in MessageBody

0ccaf39

Fix EncoderBody enum to align with Body::Message

05a606b

Fixed condition for finishing transfer of response

8f39df7

robjtede approved these changes Feb 10, 2020

View reviewed changes

Merge pull request actix#1347 from actix/bye-travis

4484b3f

Use Actions fully

JohnTitor added 2 commits February 14, 2020 07:31

Disable coverage for PRs

a1835d6

Merge pull request actix#1354 from actix/JohnTitor-patch-1

9b3f724

Disable coverage for PRs

JohnTitor approved these changes Feb 15, 2020

View reviewed changes

actix-http/Cargo.toml Outdated Show resolved Hide resolved

dunnock and others added 6 commits February 15, 2020 17:26

rollback actix-http version change

e5f116c

Remove descriptions about undefined uds feature from docs (actix#1356)

6ab7cfa

trigger retest

b4d837b

Merge branch 'master' of https://github.com/actix/actix-web into body…

356b9b4

…stream-1321

use mem::replace instead of mem::take rust 1.40+

d2388a8

point actix-http tests to ./actix-http subdir

7371142

JohnTitor approved these changes Feb 17, 2020

View reviewed changes

JohnTitor merged commit 1026d4e into actix:msg-body Feb 17, 2020

dunnock deleted the bodystream-1321 branch February 18, 2020 18:25

This was referenced Feb 18, 2020

Adapt workspace crates to MessageBody::poll with Pin #1358

Merged

Reenable actix-session and actix-identity in msg-body branch #1363

Closed

Uh oh!

Conversation

dunnock commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist:

Move to another PR / tasks

Uh oh!

JohnTitor commented Jan 30, 2020

Uh oh!

dunnock commented Jan 30, 2020

Uh oh!

dunnock commented Jan 30, 2020

ApacheBenchmark

cargo bench

Uh oh!

JohnTitor commented Jan 30, 2020

Uh oh!

dunnock commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohnTitor commented Jan 31, 2020

Uh oh!

dunnock commented Jan 31, 2020

Uh oh!

Uh oh!

cdbattags left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

robjtede left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunnock commented Feb 3, 2020

Uh oh!

Aaron1011 commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunnock commented Feb 9, 2020

Uh oh!

Aaron1011 commented Feb 9, 2020

Uh oh!

dunnock commented Feb 9, 2020

Uh oh!

dunnock commented Feb 10, 2020

Uh oh!

robjtede commented Feb 10, 2020

Uh oh!

robjtede commented Feb 10, 2020

Uh oh!

cdbattags commented Feb 13, 2020

Uh oh!

Dowwie commented Feb 14, 2020

Uh oh!

JohnTitor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JohnTitor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Comments

dunnock commented Jan 29, 2020 •

edited

Loading

dunnock commented Jan 30, 2020 •

edited

Loading

robjtede left a comment •

edited

Loading

Aaron1011 commented Feb 8, 2020 •

edited

Loading