GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Queue write only after processing all buffers by jakirkham · Pull Request #445 · MagicStack/uvloop

Conversation

jakirkham commented Oct 1, 2021

Copy link

Copy Markdown

Contributor

Partially addresses ( #404 )

Delays calling _queue_write until writelines has added all buffers with _write. Should allow libuv to leveraged vectorized IO (scatter) via calling sendmsg under-the-hood.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jakirkham added 2 commits

September 8, 2021 15:31


          Queue write only after processing all buffers

c8fc56d

Allows `writelines` to leverage vectorized IO within uvloop to send
multiple buffers in one `sendmsg` call.


          Add copyright header

a12214a

jakirkham commented

Oct 1, 2021

View reviewed changes

Comment thread

uvloop/handles/stream.pyx Resolved

Uh oh!

There was an error while loading. Please reload this page.

jakirkham mentioned this pull request

Oct 1, 2021

Using vectorized IO (scatter/gather) #404

Open

jakirkham commented Oct 1, 2021

Copy link

Copy Markdown

Contributor Author

Also given GitHub's new policy regarding running CI. Think someone needs to approve running CI here for that to happen

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Drop file copyright and update license copyright

4cd84a3

jakirkham requested a review from 1st1

October 5, 2021 18:40

jakirkham commented Oct 21, 2021

Copy link

Copy Markdown

Contributor Author

@1st1 any thoughts here?

Also need some help running the tests. GH doesn't run tests for new contributors on any commit without approval by default

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakirkham mentioned this pull request

Oct 21, 2021

Use asyncio for TCP/TLS comms dask/distributed#5450

Merged

jakirkham commented Oct 22, 2021

Copy link

Copy Markdown

Contributor Author

@fantix, do you have any thoughts on this one? 🙂

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakirkham commented Nov 5, 2021

Copy link

Copy Markdown

Contributor Author

Friendly nudge 😉

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fantix commented Aug 13, 2022

Copy link

Copy Markdown

Member

Thanks for the PR and sorry for the long waiting!

Should allow libuv to leveraged vectorized IO (scatter) via calling sendmsg under-the-hood.

Hmm do you have some test results to show if gathered send is/not working before/after this change? I'm asking because I think _queue_write() doesn't call _exec_write() until the next loop iteration, so calling _queue_write() multiple times in a tight loop in writelines() is equivalent to calling it just once. I'll run a test later when I come back to this one.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1 approved these changes

Aug 23, 2022

View reviewed changes

1st1 left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@fantix looks good. Please check that we call _queue_write whenever we call _write, but overall this is good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 commented Aug 23, 2022

Copy link

Copy Markdown

Member

Hmm do you have some test results to show if gathered send is/not working before/after this change? I'm asking because I think _queue_write() doesn't call _exec_write() until the next loop iteration, so calling _queue_write() multiple times in a tight loop in writelines()

Yeah, this has to be benchmarked, ideally, but the change still looks good & logical. I think we should go with it.

Perhaps we should reshape it a little -- add a default arg "queue_write=True" to _write and call it with "False" from writelines() (to make this internal API a bit easier to use)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakirkham commented Aug 23, 2022

Copy link

Copy Markdown

Contributor Author

Thanks all! 🙏

Yeah agree benchmarking makes sense. Is there a preferred set of benchmarks here we should try with initially? Also is there somewhere to add new benchmarks?

As to context about this change, did a fair bit of sleuthing in issue ( #404 ), which included a bit of digging into libuv itself. Could always be missing something. So would certainly appreciate feedback on that.

Sure happy to tweak the API to make things a bit more flexible.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fantix added 2 commits

September 9, 2022 12:42


          Merge branch 'master' into queue_write_once

bb3a42d


          Buffer all data first before send in writelines()

3e2016f

fantix reviewed

Sep 9, 2022

View reviewed changes

Comment thread

uvloop/handles/stream.pyx Resolved

Uh oh!

There was an error while loading. Please reload this page.

fantix requested a review from 1st1

September 9, 2022 17:47

1st1 reviewed

Sep 9, 2022

View reviewed changes

Comment thread

uvloop/handles/stream.pyx Resolved

Uh oh!

There was an error while loading. Please reload this page.

1st1 approved these changes

Sep 9, 2022

View reviewed changes


          CRF: rename and add comments

bfe9b80

fantix merged commit 9c6ecb6 into MagicStack:master

Sep 9, 2022

fantix commented Sep 9, 2022

Copy link

Copy Markdown

Member

@jakirkham thank you for starting this anyways! Please feel free to comment on the PR or the original issue.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fantix added a commit that referenced this pull request

Sep 9, 2022


          CRF: rename _start_write() to _initiate_write()

cdab9d3

Tweaked the comments too.

Refs #445

fantix added a commit that referenced this pull request

Sep 13, 2022


          uvloop v0.17.0

c4c3f9f

This release adds Python 3.11 support, updates bundled libuv to 1.43.0
and fixes a handful of issues.

Changes
=======

* Expose uv_loop_t pointer for integration with other C-extensions (#310)
  (by @pranavtbhat in b332eb8 for #310)

* Support python 3.11+ (#473)
  (by @zeroday0619 in 8e42921 for #473)

* Expose libuv uv_fs_event functionality (#474)
  (by @jensbjorgensen @fantix in 74d381e for #474)

* Activate debug mode when `-X dev` is used
  (by @jack1142 in 637a77a)

* Expose uv_version() for libuv API compatibility (#491)
  (by @fantix in 089f6cb for #491)

* Fix loop.getaddrinfo() and tests (#495)
  (by @fantix in 598b16f for #495)

* Bump to libuv 1.43.0
  (by @fantix in 94e5e53)

Fixes
=====

* _TransProtPair is no longer defined in asyncio.events
  (by @jensbjorgensen in fae5f7f)

* use a TypeVar for asyncio.BaseProtocol (#478)
  (by @graingert in 3aacb35 for #478)

* Fix segfault in TimerHandle.when() after cleared
  (by @jensbjorgensen in c39afff for #469)

* Avoid self._errpipe_write double close (#466)
  (by @graingert in 72140d7 for #466)

* Fix typo in test (#456)
  (by @kianmeng in 033d52d for #456)

* Fix potential infinite loop (#446)
  (by @kfur in ada43c0 for #446)

* use a stack of self._fds_to_close to prevent double closes (#481)
  (by @graingert in 3214cf6 for #481)

* Fix incorrect main thread id value forking from a thread  (#453)
  (by @horpto @fantix in e7934c8 for #453)

* create_subprocess_exec should treat env={} as empty environment (#439) (#454)
  (by @byllyfish in e04637e for #439)

* Queue write only after processing all buffers (#445)
  (by @jakirkham @fantix in 9c6ecb6 for #445)

* Drop Python 3.6 support for thread ident
  (by @fantix in 9c37930)

* bugfix: write to another transport in resume_writing() fails (#498)
  (by @fantix in d2deffe for #498)

Build
=====

* Upgrade GitHub Actions (#477) (#480)
  (by @cclauss in fcbf422 for #477, 1008694 for #480)

* typo `same as same`
  (by @YoSTEALTH in fedba80)

* setup.py: allow to override extra_compile_args (#443)
  (by @giuliobenetti in a130375 for #443)

* Drop hack in setup.py in finalize_options (492)
  (by @fantix in 2f1bc83 for #492)

* Fix tests invocation on release CI worklow (#489)
  (by @ben9923 in d6a2b59 for #489)

Documentation
=============

* use asyncio.Runner loop_factory on 3.11+ (#472)
  (by @graingert in 31ba48c for #472)

* Fix CI badge in docs, remove remaining Travis CI references from docs
  (by @Nothing4You in c6901a7)

* Fix typo in README
  (by @monosans in 73d7253)

fantix mentioned this pull request

Sep 13, 2022

uvloop v0.17.0 #501

Merged

fantix added a commit that referenced this pull request

Sep 14, 2022


          uvloop v0.17.0

afb3268

This release adds Python 3.11 support, updates bundled libuv to 1.43.0
and fixes a handful of issues.

Changes
=======

* Expose uv_loop_t pointer for integration with other C-extensions (#310)
  (by @pranavtbhat in b332eb8 for #310)

* Support python 3.11+ (#473)
  (by @zeroday0619 in 8e42921 for #473)

* Expose libuv uv_fs_event functionality (#474)
  (by @jensbjorgensen @fantix in 74d381e for #474)

* Activate debug mode when `-X dev` is used
  (by @jack1142 in 637a77a)

* Expose uv_version() for libuv API compatibility (#491)
  (by @fantix in 089f6cb for #491)

* Fix loop.getaddrinfo() and tests (#495)
  (by @fantix in 598b16f for #495)

* Bump to libuv 1.43.0
  (by @fantix in 94e5e53)

Fixes
=====

* _TransProtPair is no longer defined in asyncio.events
  (by @jensbjorgensen in fae5f7f)

* use a TypeVar for asyncio.BaseProtocol (#478)
  (by @graingert in 3aacb35 for #478)

* Fix segfault in TimerHandle.when() after cleared
  (by @jensbjorgensen in c39afff for #469)

* Avoid self._errpipe_write double close (#466)
  (by @graingert in 72140d7 for #466)

* Fix typo in test (#456)
  (by @kianmeng in 033d52d for #456)

* Fix potential infinite loop (#446)
  (by @kfur in ada43c0 for #446)

* use a stack of self._fds_to_close to prevent double closes (#481)
  (by @graingert in 3214cf6 for #481)

* Fix incorrect main thread id value forking from a thread  (#453)
  (by @horpto @fantix in e7934c8 for #453)

* create_subprocess_exec should treat env={} as empty environment (#439) (#454)
  (by @byllyfish in e04637e for #439)

* Queue write only after processing all buffers (#445)
  (by @jakirkham @fantix in 9c6ecb6 for #445)

* Drop Python 3.6 support for thread ident
  (by @fantix in 9c37930)

* bugfix: write to another transport in resume_writing() fails (#498)
  (by @fantix in d2deffe for #498)

Build
=====

* Upgrade GitHub Actions (#477) (#480)
  (by @cclauss in fcbf422 for #477, 1008694 for #480)

* typo `same as same`
  (by @YoSTEALTH in fedba80)

* setup.py: allow to override extra_compile_args (#443)
  (by @giuliobenetti in a130375 for #443)

* Drop hack in setup.py in finalize_options (492)
  (by @fantix in 2f1bc83 for #492)

* Fix tests invocation on release CI worklow (#489)
  (by @ben9923 in d6a2b59 for #489)

Documentation
=============

* use asyncio.Runner loop_factory on 3.11+ (#472)
  (by @graingert in 31ba48c for #472)

* Fix CI badge in docs, remove remaining Travis CI references from docs
  (by @Nothing4You in c6901a7)

* Fix typo in README
  (by @monosans in 73d7253)

jakirkham commented Sep 26, 2022

Copy link

Copy Markdown

Contributor Author

Thanks all for improving this and including it 🙏

Sorry for being unresponsive (was out at the time). Though am happy to see this conclusion while catching up 🙂

fantix reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakirkham deleted the queue_write_once branch

September 26, 2022 08:53

Reviewers

1st1

1st1 approved these changes

fantix

fantix left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.