Idiomaticity versus Performance

[formal ingot]

If it is helpful, I'm only going to callresolve once.

[lusty fulcrum]

just a general remark: if you haven't already done so, it's a good idea to profile your code to see what the performance bottlenecks are, and benchmark those bottlenecks with both approaches to see how big the difference actually is. generally speaking, I favor code readability/idiomaticity for things that aren't bottlenecks, and performance for bottlenecks

[winged mantle]

This seems like it should be a method on client. I'm also thinking async, that way the timeout could happen at any point and not just at the end or beginning of a loop. ppl don't normally include type information in labels consider src: (Ipv4Addr, MacAddr) or define a type specific for when you have this pair(Addr). That way you could return an Addr and I'm guessing you'd want to pass that pair around. Another idea is to pass back the datagram(a type implementing a custom RawSendable trait) and do the calling into your channels(raw socket?) in the caller.

[formal ingot]

My client is doing the latter thing you mentioned, this function is merely a helper to manage all of that.

The tuple advice is true IMO, I'll do that.

Is turning my eth_tx and eth_rx fields in the client into a tuple good, too, so?

[formal ingot]

And is it advisable to turn

pub fn new(
        src_ip_addr: Ipv4Addr,
        src_mac_addr: MacAddr,
        dest_ip_addr: Ipv4Addr,
        dest_mac_addr: MacAddr,
        operation: ArpOperation,
)

into

pub fn new(src: (Ipv4Addr, MacAddr), dest: (Ipv4Addr, MacAddr), op: ArpOperation) 

too, so?

or is it generally unadvised to pass tuples like so to constructors?

[formal ingot]

I haven't benchmarked yet since it isn't complete.

[prime aspen]

no that's perfectly fine

[formal ingot]

But it would be much slower, yes, due to heap alloc

[prime aspen]

... as long as the inner types are distinct.

[formal ingot]

wdym

[prime aspen]

re tuples

[winged mantle]

This is one option, but for as much as you've used it now type seems more appropriate.

[formal ingot]

wait sorry i understand

[formal ingot]

i see

thank you for this advise, i'm getting back into rust after residing in c land for a year so it is helpful

[winged mantle]

I was thinking of fn build_resolve(target: Ipv4Addr, src: (Ipv4Addr, MacAddr)) -> impl RawSendable {...}

Where RawSendable has a function like to_buffer that is passed to Client for sending.

That isolates the dirty work from the "other" dirty work.

Construction of the packet and managing send/recv.

It's just another option.

[formal ingot]

It isn't a bad idea, but I don't see how I could do async in this project, so I don't know how useful it would be to suddenly make stuff async when it doesn't really need be, nor could be by nature of itself, if you understand what I mean.

But I could be wrong: what do you think?

[winged mantle]

Depends on how you are getting a raw socket... if that's even what u do?

[formal ingot]

I'm using pnet.

Which isn't an async netstack

so it doesn't support async and from a quick search just now people who've tried to abuse it in such a way have failed

i'm stuck with these options and i'm inclined toward the present state of things even if it is ugly af

[winged mantle]

Are the timeout's set when you create the socket? https://docs.rs/pnet/latest/pnet/datalink/struct.Config.html

[formal ingot]

nah

those are timeouts for trying to write and read to the channel as far as i understand

[winged mantle]

This is where I'd imagine the timeout parameter you include would be used: https://docs.rs/pnet/latest/pnet/datalink/trait.DataLinkReceiver.html

[formal ingot]

nah. you get a rx from datalink::channel which takes an interface and the aforementioned config

the read timeout would apply, yep

[formal ingot]

but in this sense

this prevents trying to read a large packet

in scenarios where, say, you're deliberately sent a gigantic packet for denial of service

[winged mantle]

I think that config struct is the only chance you have to specify a timeout. Then it depends on what you are putting into Client, if it's the DataLinkReceiver and DataLinkSender as I'd guess then I don't see where you'd use the timeout value.

Sorry, but yes async is not an option.

[formal ingot]

timeout value is for me to implement waiting for a reply

timeout in the context of tx and rx is how long to write and read before stopping

timeout in the context of my code is how long to wait before acc reading

do you understand?

btw just want to thank you for the help i'm spamming this server w questions for the past two days just to get my rust brain going again 😭

[winged mantle]

np, I enjoy this kind of thing 🙂

[winged mantle]

This is the part I thought I understood until I read your other statements... I'll have to think about them, but for a moment I'll theory craft.

The waiting for a reply part should have a timeout and you could set the timeout in Config low and try and recv in a loop until a "soft timer" expires... don't do that.

What I might do is set the Config timeout to 1/3rd my actual timeout and try recv 3 times, that way you can print something like "Waiting for remote host...".

What's "acc reading"?

I'm taking "in the context of tx and rx" to mean the pnet Config settings.

[formal ingot]

yep

[formal ingot]

reading pkts from the receiver

[formal ingot]

not a bad idea

[winged mantle]

When I saw you sending the timeout down your call chain my thought was you wanted the code to be async and use the timeout feature of whatever async engine you chose. That's how it normaly works.

[formal ingot]

i would if i could use async. i think i'll have to do the forbidden "put the thread to sleep" though this should be okay as the client doesn't internally queue packets received, you read from the actual rx on the actual interface when you want to.

[winged mantle]

That code like this returns something like a Future and the calling code acts on that future specifying the timeout in a second call.

[formal ingot]

and the pkt will send before sleeping occurs.

[winged mantle]

Even if you don't use an async future, I'd still suggest modeling the code that way.

[formal ingot]

i'll brush up on futures and do so.

thank you.

[formal ingot]

what about calling the recv fn on another thread

[old furnace]

I'm usually all for performance. But in this case, arp happens rarely. On the order of once a second and then never again for a long long while. Go for clarity.

You need an arp cache to hold responses.

Worth noting that a, the actual resolving can take a while and b, the thing requesting the resolve can be deleted before the resolution is complete.

Also, arp packets are tiny.

[formal ingot]

I'll refractor tomorrow, thanks.

[winged mantle]

@old furnace Another thread because the read call is blocking in this case.

[winged mantle]

I'm not sure that helps "with the original problem", I always prefer forks over threads so I don't know what control the other thread would have "when inside the read call". You can use () with a std::sync::mpsc and loop over reading and checking the channel, but avoid using a small timeout. You'll also want to use the mpsc to send the response. I don't know about joining the thread handle, just have no experience. I would guess rust does something sane if the handle goes out of scope.

Having another thread do the recv 3 times would work, just think about what work you'd do in the other thread while waiting.

[reef obsidian]

I always prefer forks over threads
the fork syscall?

[winged mantle]

Yes, it's hard to find a use case where threads are mandatory... admitting that this is one such case.

[reef obsidian]

Interesting. Do you IPC every time you need your "threads" to communicate?

[formal ingot]

i might avoid the need for spawning x altogether and implement the timing myself in the recv method

smth like while duration - Instant::now() < duration { ... }

seems to be the best approach

[winged mantle]

Yes... There are likely all kinds of pro and con arguments, but as I've not looked into threads I'm not able to comment.