Rewrite ::address (import from dbus_addr) #989
Conversation
|
Thanks! I'll have a look soon. In the meantime, could you please rebase this on top of #976? That one is just waiting for a test case to be added. |
|
I mainly see only 2 blockers here now:
|
I disagree with you on those two points, but if it is what it takes to make progress... |
That's well established. :)
I'm sorry but yeah. You've to keep in mind that I did try my best to explain my reasoning in detail. I regret that I failed to convince you in the end. |
elmarco
force-pushed
the
address
branch
6 times, most recently
from
a0f3be2
to
8b9f688
Compare
elmarco
changed the title
|
@elmarco is this ready for review? |
yes please |
|
|
||
| fn validate(&self) -> Result<()> { | ||
| self.transport()?; | ||
| let mut set = HashSet::new(); |
There was a problem hiding this comment.
Wait, aren't we always allocating then for validation (which always happens)? 🤔 The address strings are typically not that long and typically (99% at least IMO) only parsed once so if the API is designed to avoid allocations, it should at least reduce allocations compared to the existing API.
This was my thinking when I decided not to cater for avoiding allocations in 4.0 address API in the end: if in the end I still have allocations, what's the point?
There was a problem hiding this comment.
I wrote this code the way I thought would be more efficient for parsing, but I never actually went the whole route and checked against nostd etc. I think it's in general superior than always using Vec and String all over. But yes, as I told you earlier, there should be an escape solution for assuming an address is already validated, and/or a way to use no_std (hashset could be replaced quite trivially by a vec from alloc, or a fixed array) . Although having this merged in zbus is now harder to work on and guaratee (same as guarateeing the namespace remain clear of other internal dependencies..).
There was a problem hiding this comment.
I wrote this code the way I thought would be more efficient for parsing, but I never actually went the whole route and checked against nostd etc.
I think you mean noalloc but that's not what we need. It'll be a pretty big challenge to make zbus nostd and especially noalloc. I'm not expecting that at all from this API. I have a vague plan of adding nostd and noalloc features through the use of heapless crate but that would be a pretty big project and not relevant here.
I think it's in general superior than always using Vec and String all over.
In general, yes and that's my point: is the new API really reducing allocations if it always allocate for validation?
But yes, as I told you earlier, there should be an escape solution for assuming an address is already validated,
I disagree. An address should always be validated, unless built through an unsafe API. This goes in line with the new signature API: zvariant::Signature is now always valid. I don't think this is a strong argument.
and/or a way to use no_std (hashset could be replaced quite trivially by a vec from alloc, or a fixed array)
nostd is not the target here. Reduction/avoidance of allocations is, since that's one of the main arguments for the new API here.
Although having this merged in zbus is now harder to work on and guaratee (same as guarateeing the namespace remain clear of other internal dependencies..).
Not sure I understand but let's focus here: Does the new API proposed here, reduces allocations and if so, how much and if it's worth having all the lifetimes and other complicated parts etc.
There was a problem hiding this comment.
Not sure I understand but let's focus here: Does the new API proposed here, reduces allocations and if so, how much and if it's worth having all the lifetimes and other complicated parts etc.
Yes if you skip validation (my intention earlier was obviously behind unsafe.) but I don't think that's the main argument for using this code anyway, almost nobody will care about that, especially if it can't be used directly without the rest of zbus. parsing and and storing result is done through &str references whenever possible.
There has been a lot of reasons listed earlier, if you wish me to make the list again, I could Not to mention it was written before zbus ::address rewrite, which is fundamental in its existence, otherwise, I might have just done something else instead...
There was a problem hiding this comment.
also hashset usage is really extra, there is nothing that really prevents addresses from having duplicated keys.. It just seems wrong, because the behaviour isn't described. So if we drop that check, we have no hashset, if that's what worries you about this code.
There was a problem hiding this comment.
and the lifetime usage is one of the most trivial Rust could offer, it references the given string lifetime. Can't be much simpler, you can agree.
There was a problem hiding this comment.
Yes if you skip validation (my intention earlier was obviously behind unsafe.)
But why would we want to skip validation at all? That makes no sense to me to bring that up as an argument.
but I don't think that's the main argument for using this code anyway
It has been one of your main arguments, sorry. That and support for address list, which we already established is not something anyone really misses. When I told you that AddressList could simply be a Vec<Address>, your only argument against that was that it will force allocation on people. So forcing tiny amount of allocation on use of an extremely niche API, is a big deal but forcing it on everyone is not?
almost nobody will care about that
That's exactly my point.
especially if it can't be used directly without the rest of zbus.
I told you how you can make that happen, without a lot of effort using cargo features. In fact you can make use of the API break right now to make it happen already in 5.0.
There has been a lot of reasons listed earlier, if you wish me to make the list again
This is the only advantage I see in your list that would require an intrusive re-write of the API that you're proposing. All others could be addressed with the current API.
also hashset usage is really extra, there is nothing that really prevents addresses from having duplicated keys.. It just seems wrong, because the behaviour isn't described. So if we drop that check, we have no hashset, if that's what worries you about this code.
It doesn't worry me one bit. It just contradicts your main argument here.
Anyway, if the spec doesn't dictate this, then we shouldn't do this. In case of duplicate keys, let's just take the last value and document this behaviour?
There was a problem hiding this comment.
Anyway, if the spec doesn't dictate this, then we shouldn't do this. In case of duplicate keys, let's just take the last value and document this behaviour?
If you agree to this, I believe we can merge this once you've addressed this.
The following commits introduce a different implementation.
Convert to legacy Error::Address string error.
Move connection logic to connect.rs. We start moving parsing to the new ::address step by step, here with top-level list. legacy_address is going to be gradually replaced.
Compared to the existing implementation, this will take the family into account (except with tokio), as well as multiple host resolution.
Parsing can be done with legacy Address and DBusAddr
Connecting to an address can recursively resolve to a different address to connect to. Because it's an async function, box it.
As requested by Zeeshan.
Based on the discussion from #982, import dbus_addr as zbus_address and rebase #562 to make use of it.