-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: Add support for more packages to DeepSize #18
feat: Add support for more packages to DeepSize #18
Conversation
@matklad Please, let me know whenever it's a good idea to split this change.
|
For array sizes, #16 is already split off. I guess the rest looks fine, but thats for @Aeledfyr to decide :) I would probably suggest to refactor the code a bit to make sure that per-crate impls live in crate-specific file (ie, alloc.rs, std.rs, tokio.rs, actix.rs), but that is orthogonal to adding the impls at all. |
This looks good. One issue though is with adding feature dependencies: Line 23 in 90a1efc
By adding a dependency on tokio with the full feature set, I believe that we enable that feature set in all crates that depend on deepsize , even if they don't enable the tokio feature in deepsize .
Is there any way to properly deal with this? For example, it might be possible to have a |
Splitting the Also, most of those implementations could use the deepsize/src/external_impls.rs Lines 167 to 172 in 23065db
|
@Aeledfyr Can you take a look again? |
There are still some issues with dependency resolution. This PR fails to compile with Also, the I think the cleanest way to solve the As a basic example of how I think it could work: [dependencies]
tokio = { version = "^1.1", optional = true, default-features = false }
# default-features is technically unnecessary here as tokio
# has no default features, but adding it is good practice for
# a library
# ...
[features]
tokio_net = ["tokio", "tokio/net"]
# ... #[cfg(feature = "tokio_net")]
mod tokio_net_impl {
// ...
} I haven't verified that this works, but it seems like it should. I also haven't verified that this won't lead to accidentally enabling features in dependant's crates, but from what I can tell this requires the feature to be explicitly enabled for the |
@Aeledfyr Yes, it works. Thanks. |
@Aeledfyr I updated the PR, can you take a look? |
Sorry about the delay; I believe the |
@miguelfrde Thanks, I updated the PR.
|
I can't verify that the feature resolution works entirely properly, but from |
We want to track of sizes of messages sent from `PeerActor` to `PeerManagerActor`. We will stop reading from given `PeerActor` once we reach the limit. To achieve this a new type `ActixMessageWrapper` was introduced. It wraps around messages sent to peer, and contains metadata needed for stat counting. This is a draft: - [x] Add a wrapper for sending messages from`PeerActor` to `PeerManagerActor` - [x] Modify `PeerActor` to use - [x] Add a wrapper for reply's to the above messages (wating for `DeepSizeOf`) - [ ] Wait for `DeepSizeOf` (another PR) - [ ] Add a wrapper for messages to Peer - for example for routing (another PR) - [ ] Add a wrapper for messages to EdgeVerifierActor - for example for routing (another PR) - [ ] Add a wrapper for messages to RoutingTableActor - for example for routing (another PR) - [ ] Add a wrapper for messages to ClientActor - for example for routing (another PR) - [ ] Extensions to `performance_stats` to track per thread limits (another PR) - [ ] Extension to track bandwidth (another PR) This PR is based on, not yet merged, #4927 Resolves #5155 Blocked by Aeledfyr/deepsize#18
We want to track of sizes of messages sent from `PeerActor` to `PeerManagerActor`. We will stop reading from given `PeerActor` once we reach the limit. To achieve this a new type `ActixMessageWrapper` was introduced. It wraps around messages sent to peer, and contains metadata needed for stat counting. This is a draft: - [x] Add a wrapper for sending messages from`PeerActor` to `PeerManagerActor` - [x] Modify `PeerActor` to use - [x] Add a wrapper for reply's to the above messages (wating for `DeepSizeOf`) - [ ] Wait for `DeepSizeOf` (another PR) - [ ] Add a wrapper for messages to Peer - for example for routing (another PR) - [ ] Add a wrapper for messages to EdgeVerifierActor - for example for routing (another PR) - [ ] Add a wrapper for messages to RoutingTableActor - for example for routing (another PR) - [ ] Add a wrapper for messages to ClientActor - for example for routing (another PR) - [ ] Extensions to `performance_stats` to track per thread limits (another PR) - [ ] Extension to track bandwidth (another PR) This PR is based on, not yet merged, #4927 Resolves #5155 Blocked by Aeledfyr/deepsize#18
We want to track of sizes of messages sent from `PeerActor` to `PeerManagerActor`. We will stop reading from given `PeerActor` once we reach the limit. To achieve this a new type `ActixMessageWrapper` was introduced. It wraps around messages sent to peer, and contains metadata needed for stat counting. This is a draft: - [x] Add a wrapper for sending messages from`PeerActor` to `PeerManagerActor` - [x] Modify `PeerActor` to use - [x] Add a wrapper for reply's to the above messages (wating for `DeepSizeOf`) - [ ] Wait for `DeepSizeOf` (another PR) - [ ] Add a wrapper for messages to Peer - for example for routing (another PR) - [ ] Add a wrapper for messages to EdgeVerifierActor - for example for routing (another PR) - [ ] Add a wrapper for messages to RoutingTableActor - for example for routing (another PR) - [ ] Add a wrapper for messages to ClientActor - for example for routing (another PR) - [ ] Extensions to `performance_stats` to track per thread limits (another PR) - [ ] Extension to track bandwidth (another PR) This PR is based on, not yet merged, #4927 Resolves #5155 Blocked by Aeledfyr/deepsize#18
I would like to add support for a few types from
tokio
,actix
.use tokio::net::TcpStream
use actix::Addr