Rewrite libuv bindings for efficiency and ~fn() removal #10321
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Stellar effort. 🌞 |
This was referenced Nov 7, 2013
bors
added a commit
that referenced
this pull request
Nov 8, 2013
The major impetus for this pull request was to remove all usage of `~fn()` in `librustuv`. This construct is going away as a language feature, and additionally it imposes the requirement that all I/O operations have at least one allocation. This allocation has been seen to have a fairly high performance impact in profiles of I/O benchmarks. I've migrated `librustuv` away from all usage of `~fn()`, and at the same time it no longer allocates on every I/O operation anywhere. The scheduler is now much more tightly integrated with all of the libuv bindings and most of the uv callbacks are specialized functions for a certain procedure. This is a step backwards in terms of making `librustuv` usable anywhere else, but I think that the performance gains are a big win here. In just a simple benchmark of reading/writing 4k of 0s at a time between a tcp client/server in separate processes on the same system, I have witnessed the throughput increase from ~750MB/s to ~1200MB/s with this change applied. I'm still in the process of testing this change, although all the major bugs (to my knowledge) have been fleshed out and removed. There are still a few spurious segfaults, and that's what I'm currently investigating. In the meantime, I wanted to put this up for review to get some eyes on it other than mine. I'll update this once I've got all the tests passing reliably again.
|
From auto-win-32-opt log: This indicates |
Using an raii wrapper means that there's no need for a '_self' variant and we can greatly reduce the amount of 'self_'-named variables.
This cleans up the merging of removing ~fn() and removing C++ wrappers to a compile-able and progress-ready state
This exposes the ability to change the modification and access times on a file. Closes rust-lang#10266
It turns out that the uv implementation would cause use-after-free if the idle callback was used after the call to `close`, and additionally nothing would ever really work that well if `start()` were called twice. To change this, the `start` and `close` methods were removed in favor of specifying the callback at creation, and allowing destruction to take care of closing the watcher.
In the ideal world, uv I/O could be canceled safely at any time. In reality, however, we are unable to do this. Right now linked failure is fairly flaky as implemented in the runtime, making it very difficult to test whether the linked failure mechanisms inside of the uv bindings are ready for this kind of interaction. Right now, all constructors will execute in a task::unkillable block, and all homing I/O operations will prevent linked failure in the duration of the homing operation. What this means is that tasks which perform I/O are still susceptible to linked failure, but the I/O operations themselves will never get interrupted. Instead, the linked failure will be received at the edge of the I/O operation.
It appears that uv's support for interacting with a stdio stream as a tty when it's actually a pipe is pretty problematic. To get around this, promote a check to see if the stream is a tty to the top of the tty constructor, and bail out quickly if it's not identified as a tty. Closes rust-lang#10237
At this time, also point the libuv submodule to the official repo instead of my own off to the side. cc rust-lang#10246 Closes rust-lang#10329
When a channel is destroyed, it may attempt scheduler operations which could move a task off of it's I/O scheduler. This is obviously a bad interaction, and some finesse is required to make it work (making destructors run at the right time). Closes rust-lang#10375
bors
added a commit
that referenced
this pull request
Nov 10, 2013
The major impetus for this pull request was to remove all usage of `~fn()` in `librustuv`. This construct is going away as a language feature, and additionally it imposes the requirement that all I/O operations have at least one allocation. This allocation has been seen to have a fairly high performance impact in profiles of I/O benchmarks. I've migrated `librustuv` away from all usage of `~fn()`, and at the same time it no longer allocates on every I/O operation anywhere. The scheduler is now much more tightly integrated with all of the libuv bindings and most of the uv callbacks are specialized functions for a certain procedure. This is a step backwards in terms of making `librustuv` usable anywhere else, but I think that the performance gains are a big win here. In just a simple benchmark of reading/writing 4k of 0s at a time between a tcp client/server in separate processes on the same system, I have witnessed the throughput increase from ~750MB/s to ~1200MB/s with this change applied. I'm still in the process of testing this change, although all the major bugs (to my knowledge) have been fleshed out and removed. There are still a few spurious segfaults, and that's what I'm currently investigating. In the meantime, I wanted to put this up for review to get some eyes on it other than mine. I'll update this once I've got all the tests passing reliably again.
Jarcho
pushed a commit
to Jarcho/rust
that referenced
this pull request
Feb 26, 2023
Fix false positives for `extra_unused_type_parameters` Don't lint external macros. Also, if the function body is empty, any type parameters with bounds on them are not linted. Note that only the body needs be empty - this rule still applies if the function takes any arguments. fixes rust-lang#10318 fixes rust-lang#10319 changelog: none <!-- changelog_checked -->
Jarcho
pushed a commit
to Jarcho/rust
that referenced
this pull request
Feb 26, 2023
Fix more false positives for `extra_unused_type_parameters` Builds on rust-lang#10321. All empty functions are no longer linted, instead of just those that have trait bounds on them. Also, if a trait bound contains a non-public trait (un-exported, but still potentially reachable), then the corresponding type parameter isn't linted. Finally, added support for the `avoid_breaking_exported_api` config option. r? `@flip1995` changelog: none
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The major impetus for this pull request was to remove all usage of
~fn()inlibrustuv. This construct is going away as a language feature, and additionally it imposes the requirement that all I/O operations have at least one allocation. This allocation has been seen to have a fairly high performance impact in profiles of I/O benchmarks.I've migrated
librustuvaway from all usage of~fn(), and at the same time it no longer allocates on every I/O operation anywhere. The scheduler is now much more tightly integrated with all of the libuv bindings and most of the uv callbacks are specialized functions for a certain procedure. This is a step backwards in terms of makinglibrustuvusable anywhere else, but I think that the performance gains are a big win here.In just a simple benchmark of reading/writing 4k of 0s at a time between a tcp client/server in separate processes on the same system, I have witnessed the throughput increase from ~750MB/s to ~1200MB/s with this change applied.
I'm still in the process of testing this change, although all the major bugs (to my knowledge) have been fleshed out and removed. There are still a few spurious segfaults, and that's what I'm currently investigating. In the meantime, I wanted to put this up for review to get some eyes on it other than mine. I'll update this once I've got all the tests passing reliably again.