portfwd: Optimize errors.Is() checks

[nirs]

On the read data path, we had many checks like:

if errors.Is(err, io.EOF) {
    return nil 
}   

There are 2 issues with these checks:

1. io.Copy() is documented to never return io.EOF, and current code
   matches the docs.
2. errors.Is() is expensive and should not be used in the fast path

This change remove the checks when they are not needed, and it the case
of handling Recv() which is documented to return io.EOF, or
PacketCon.ReadFrom() which does not mention io.EOF semantics, move the
check into a err != nil block.

I did not make any measurements with lima, but dolthub.com did synthetic
benchmarks:
https://www.dolthub.com/blog/2024-05-31-benchmarking-go-error-handling/

Based on the benchmarks the difference in the case when err == nil is
not big, but the code is more clear when we gather all error handling
under a err != nil check.

[tamird]

This can't possibly matter and serves only to introduce a conflict with #2944. Some of these conditions are statically impossible as I stated in #2944.

[nirs]

This can't possibly matter

Did you do any measurements?

Some of these conditions are statically impossible as I stated in #2944.

What is impossible? see my comment here: 553ebd9#r1868342753

[nirs]

I'll change this to remove all the io.EOF checks, as @tamird pointed out that ReadFrom is documented not to the return io.EOF, and io.Copy() implementation doesn't return it.

[tamird]

This can't possibly matter

Did you do any measurements?

Here are some of the results from the blog post you linked:

ErrEqual | 7.366 ns/op | 2.15
ErrEqualNilCheck | 8.293 ns/op | 2.42

ErrorsIs | 19.35 ns/op | 5.65
ErrorsIsNilCheck | 19.34 ns/op | 5.65

So the nil check saves approximate 1 nanosecond. These code paths are invoked once per socket connection. Does one nanosecond matter?

Some of these conditions are statically impossible as I stated in #2944.

What is impossible? see my comment here: 553ebd9#r1868342753

This is a dead link, but I replied to your comment: #2944 (comment)

[alexandear]

We can simplify this further by completely removing errors.Is(err, io.EOF):

Suggested change

	if err != nil && errors.Is(err, io.EOF) {
	return nil
	}
	return err
	return err

According to the doc, io.Copy never returns io.EOF:

Copy copies from src to dst until either EOF is reached on src or an error occurs. It returns the number of bytes copied and the first error encountered while copying, if any.

A successful Copy returns err == nil, not err == EOF. Because Copy is defined to read from src until EOF, it does not treat an EOF from Read as an error to be reported.

[nirs]

Yes, the check was removed now.

[nirs]

@tamird as I wrote, errors.Is() does not make sense even if the difference is not significant. We should handle errors only if there was an error.

The change updated to remove the unneeded checks.

[tamird]

Callers should always process
// the n > 0 bytes returned before considering the error err.

https://pkg.go.dev/net#PacketConn

Seems this code is wrong.

[nirs]

Correct, this is a bug in the code, but it is not related this change.

[nirs]

Opened #2970

[nirs]

Should be fixd by #2971, thanks for pointing out this isue!

[nirs]

This can't possibly matter and serves only to introduce a conflict with #2944.

Don't worry about conflicts. One of the PRs will be merged first and the other will do a trivial rebase.

[jandubois]

3. errors.Is() is expensive and should not be used in the fast path

This is simply not true. It shortcuts if either err or target are nil:

func Is(err, target error) bool {
	if err == nil || target == nil {
		return err == target
	}

	isComparable := reflectlite.TypeOf(target).Comparable()
	return is(err, target, isComparable)
}

There is no reason to avoid calling it, even if err is nil. The main concern should be code readability. If calling errors.Is saves you an extra conditional, please call it instead of trying to optimize away a function call that the compiler may have inlined anyways.

[jandubois]

Thanks, LGTM

[nirs]

3. errors.Is() is expensive and should not be used in the fast path

This is simply not true. It shortcuts if either err or target are nil:

The shortcut is still a useless function call. It makes sense in code that you don't care about, if it is more clear.

There is no reason to avoid calling it, even if err is nil. The main concern should be code readability.

Right, it make the code worse.

If calling errors.Is saves you an extra conditional, please call it instead of trying to optimize away a function call that the compiler may have inlined anyways.

I'm sure the compiler cannot inline it. We can have 1000's of these, and it does not make sense to inline such function.

Doing more work in the data path is always bad idea, even if the compiler optimize it. The entire errors.Is() thing is is a bad idea. Instead of returning sentinel error and doing a fast check, people wrap the sentinel in other errors and wasted cycles on finding the wrapped errors. Even worse, the error tree may contain a wrapped io.EOF when the error is real and should not be handled as io.EOF. io.Copy() does not the right way doing equality check on io.EOF.

[jandubois]

The shortcut is still a useless function call.

You are just manually inlining code to avoid a "useless" call. The same argument could be made if you have a function to add 2 numbers, and you write:

if b != 0 {
    a = add(a, b)
}

to avoid a "useless function call" in case b is 0.

To me replacing errors.Is(err, someError) with err != nil && errors.Is(err, SomeError) is the same kind of obfuscating micro-optimization that does not improve readability.

The new code in this PR was not of that kind, so I merged it, but I would not agree with fluffing up code by manually inlining parts of functions. That's what optimizing compilers are for.

I'm sure the compiler cannot inline it.

Why not? I know there are a bunch of conditions around inlining, but anything smaller than something like 100 AST nodes will be inlined, and Is() looks small enough for that.

[jandubois]

anything smaller than something like 100 AST nodes will be inlined

My naïve attempt shows that errors.Is is indeed not inlined, even though it seems trivial enough. I don't have time to dig deeper.

I still think the overhead of a single "useless" function call is trivial. I don't think it will be noticeable if this is on top of a system call.

We will never make back the time we wasted discussing this!