Performance regression accesing UDP header fields in 0.0.8

[aureliar8]

Commit a05adf7 changed UdpHdr fields to be [u8;2] rather than u16. Rather than directly accessing the fields, one should now use the associated getter.

However, the generated code when using the header is made of more instructions than what it was before. And on my case, it make my program go over the 1M instruction limit of the verifier.

My program iterates over a list of rule and verify if the rule is met for the packet
Here's the hot-loop body of code when using 0.0.7:

Self::InnerDstPort(port) => match pkt.inner_ulp {
    Some(Ulp::Udp(hdr)) => unsafe { (*hdr).dest.swap_bytes() == *port },
    _ => false,
},

When upgrading to 0.0.8 what contains commit a05adf7, I now need to do:

Self::InnerDstPort(port) => match pkt.inner_ulp {
    Some(Ulp::Udp(hdr)) => unsafe { (*hdr).dest() == *port };
    _ => false,
},

But now the verifier rejects my program.
Here's the result of llvm-objdump for the 0.0.7 version. It compiles to 12 instructions

;             Self::InnerDstPort(port) => match pkt.inner_ulp {
     270:	r2 = *(u64 *)(r10 - 0x98)
     271:	r2 &= 0x1
     272:	if r2 != 0x0 goto -0x48 <epte::process_pkt::h89b31b98142ab593+0x648>
;                 Some(Ulp::Udp(hdr)) => unsafe { (*hdr).dest.swap_bytes() == *port },
     273:	r2 = r3
     274:	r2 += r5
     275:	r2 = *(u16 *)(r2 + 0x4)
     276:	r0 = *(u64 *)(r10 - 0x88)
     277:	r0 = *(u16 *)(r0 + 0x2)
;             intrinsics::bswap(self as $ActualT) as Self
     278:	r0 = be16 r0
;                     if !pred.is_match(pkt) {
     279:	if r0 == r2 goto -0x2f <epte::process_pkt::h89b31b98142ab593+0x748>
     280:	goto -0x50 <epte::process_pkt::h89b31b98142ab593+0x648>
;         match self {
     281:	if r8 == 0x3 goto +0x17 <epte::process_pkt::h89b31b98142ab593+0x988>
     282:	if r8 == 0x4 goto -0x6e <epte::process_pkt::h89b31b98142ab593+0x568>

And here's the result of llvm-objdump for the 0.0.8 version. It compiles to 15 instruction

;             Self::InnerDstPort(port) => match pkt.inner_ulp {
     286:	r2 = *(u64 *)(r10 - 0x98)
     287:	r2 &= 0x1
     288:	if r2 != 0x0 goto -0x49 <epte::process_pkt::h020d55f9b8edebf0+0x6c0>
     289:	r0 = *(u64 *)(r10 - 0x88)
;                 Some(Ulp::Udp(hdr)) => unsafe { (*hdr).dest() == *port },
     290:	r2 = *(u8 *)(r0 + 0x2)
     291:	r0 = *(u8 *)(r0 + 0x3)
     292:	r0 <<= 0x8
     293:	r0 |= r2
     294:	r2 = r3
     295:	r2 += r5
     296:	r2 = *(u16 *)(r2 + 0x4)
;             intrinsics::bswap(self as $ActualT) as Self
     297:	r0 = be16 r0
;                     if !pred.is_match(pkt) {
     298:	if r0 == r2 goto -0x33 <epte::process_pkt::h020d55f9b8edebf0+0x7c0>
     299:	goto -0x54 <epte::process_pkt::h020d55f9b8edebf0+0x6c0>
;         match self {
     300:	if r8 == 0x3 goto +0x20 <epte::process_pkt::h020d55f9b8edebf0+0xa68>
     301:	if r8 == 0x4 goto -0x7a <epte::process_pkt::h020d55f9b8edebf0+0x5a0>

The differences comes from the fact that the two bytes that make the field are loaded separately and then combined with bitwise operations

Here's the workaround I used to make my program pass the verifier on 0.0.8:

Self::InnerDstPort(port) => match pkt.inner_ulp {
    Some(Ulp::Udp(hdr)) => unsafe {
        let csum = (*((hdr as usize + 2) as *const u16)).swap_bytes();
        csum == *port
    },
    _ => false,
},

This compiles to the exact same byte-code as the 0.0.7 version.

What do you think of this issue ? Should we try to change the implementation of the getter to make it load the u16 in a single instruction ?

This PR #56 likely had the same issue for other header type but I didn't test it as it isn't released yet

Related to #32

[vadorovsky]

Thanks for the report!

What do you think of this issue ? Should we try to change the implementation of the getter to make it load the u16 in a single instruction ?

Yes, it would be great to do that. We could replace all the from_be_bytes calls with the pointer arithmetic and casts. Do you want to make a PR or should I do that?

[vadorovsky]

Also, we could use https://docs.rs/memoffset/0.9.1/memoffset/macro.offset_of.html instead of hard coding offsets (like + 2 in your example). To be precise, we could change:

    #[inline]
    pub fn dst_port(&self) -> u16 {
        u16::from_be_bytes(self.dst)
    }

to:

    #[inline]
    pub fn dst_port(&self) -> u16 {
        unsafe {
            *((self as *const Self as usize + memoffset::offset_of!(Self, dst)) as *const u16)
        }
        .swap_bytes()
    }

(and add some Safety comment)

[vadorovsky]

We could also optimize setters with:

    #[inline]
    pub fn set_dst_port(&mut self, dest: u16) {
        self.dst = unsafe { *((&dest.swap_bytes() as *const _ as usize) as *const _) };
    }

[aureliar8]

I'm currently on vacation until August
But after that I can make the PR !

[vadorovsky]

I'm currently on vacation until August But after that I can make the PR !

I will go ahead and make the PR myself, I want to make a release this month. Sorry for that!

[aureliar8]

Hi @vadorovsky, I'm back from holidays.
Do you want me to work on the PR or have you already done something on your side ?

[vadorovsky]

Do you want me to work on the PR or have you already done something on your side ?

Sorry for late response. I have the code ready since some time, totally forgot about submitting it. #71