Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (1404e37) to head (0854f62).
Report is 1 commits behind head on master.

@@            Coverage Diff            @@
##            master      #206   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          548       552    +4     
=========================================
+ Hits           548       552    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Increasing the execution time by 13% for valid cases seems like a potential downside to me - not everyone's workload is the same as yours with many invalid values.

On the other hand, the actual time change is really not that much - we're talking about 0.65 ns/op - vs. the major improvement in the invalid case.

I'm leaning toward accepting this, but I'm not totally sure. Let's see what anyone else has to say.

@kohenkatz Not sure either. The speed up for the invalid cases is rather substantial, but how often in the wild would this case actually be encountered and on the happy path of a consumer's workflow? It's also a fairly specific invalid case of empty/partial string.

The sub nanosecond time increase on the valid case doesn't worry me that much. Willing to accept this trade but will probably give this a couple of weeks to see if anybody pipes up and raises a concern.

@gaffneyc would you mind adding a unit test to make sure we're at 100% coverage?

I'd lean towards a no because of the +13% increase in the valid case because there are existing consumers that generate a very large volume of valid cases (in the multiple trillions of values per day). While .65 nanoseconds is an extremely small amount of time, adding that to 3 trillion+ operations is an additional 22.5 hours of processing for every 24 hour window.

I took a look at the code coverage change and since we're checking the only case where FromBytes can return an error there was no way to test the conditional which is what caused the drop in test coverage. My intention with the original change was to make the smallest change necessary as there may be other cases in the future where FromBytes could return an error (though I can't think of any).

That said, I've pushed an alternative implementation which pulls code from FromBytes and UnmarshalBinary into FromBytesOrNil. This avoids checking the length of the input twice as well as the overhead involved with calling multiple functions but duplicates the (minimal) logic for unmarshalling from a byte slice. I've also rerun the benchmarks on a less busy system and switched to benchstat for comparison.

By the way, the extra overhead of 0.65ns across 3 trillion calls would be 32 minutes and 30 seconds rather than 22 hours and 30 minutes.

I've added the benchmarks in the main comment for the updated implementation. In the example with 3 trillion requests this would save ~2 hours and 15 minutes of compute time per day for valid UUIDs and ~112 hours and 15 minutes if all of those UUIDs were empty.

@gaffneyc I like this new version!

... but duplicates the (minimal) logic for unmarshalling from a byte slice.

I am not concerned about that, but I think there should be a comment here explaining that it has been done on purpose for performance reasons so that some future contributor (or ourselves in 6 months) doesn't try to "fix" this and consolidate it again.

@kohenkatz I've added a comment though let me know if you think it could be more clear. Thanks!

Looks reasonable to me.

Let's see if @cameracker and @dylan-bourque (or anyone else) have thoughts.

I ran the benchmarks locally and saw improvements in both time and allocs.

 benchstat before.txt after.txt
goos: darwin
goarch: arm64
pkg: github.com/gofrs/uuid/v5
cpu: Apple M1 Max
                        │   before.txt   │              after.txt               │
                        │     sec/op     │    sec/op     vs base                │
FromBytesOrNil/valid-10      5.186n ± 2%    1.790n ± 2%  -65.49% (p=0.000 n=20)
FromBytesOrNil/empty-10   115.0000n ± 1%   0.5857n ± 2%  -99.49% (p=0.000 n=20)
geomean                      24.42n         1.024n       -95.81%

                        │  before.txt  │                after.txt                │
                        │     B/op     │    B/op     vs base                     │
FromBytesOrNil/valid-10   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=20) ¹
FromBytesOrNil/empty-10   96.00 ± 0%      0.00 ± 0%  -100.00% (p=0.000 n=20)
geomean                              ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

                        │  before.txt  │                after.txt                │
                        │  allocs/op   │ allocs/op   vs base                     │
FromBytesOrNil/valid-10   0.000 ± 0%     0.000 ± 0%         ~ (p=1.000 n=20) ¹
FromBytesOrNil/empty-10   2.000 ± 0%     0.000 ± 0%  -100.00% (p=0.000 n=20)
geomean                              ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

@gaffneyc did you change something significant since this PR was first created that would have improved the behavior?

Regardless, my objection was to the ~13% increase, which doesn't seem to be manifesting anymore.

Another option here to avoid allocating errors that are just discarded would be to change UnmarshalBinary() to return a constant error rather than using fmt.Errorf()

@dylan-bourque Yes, I rewrote the implementation pulling code from FromBytes and UnmarshalBinary into FromBytesOrNil. This was the only way without changing UnmarshalBinary to avoid checking the input's length twice and to get code coverage back to 100%.

Another option here to avoid allocating errors that are just discarded would be to change UnmarshalBinary() to return a constant error rather than using fmt.Errorf()

Yeah, that could work as well though it would be a change in behavior. To keep the same behavior you could implement a custom type which would wrap the ErrIncorrectByteLength, store the invalid size, and respond with the full message when Error() is called. Not sure what impact either option would have without benchmarking it.

The original implementation was:

func FromBytesOrNil(input []byte) UUID {
	if len(input) != Size {
		return Nil
	}

	uuid, err := FromBytes(input)
	if err != nil {
		return Nil
	}
	return uuid
}

The new implementation is:

func FromBytesOrNil(input []byte) UUID {
	if len(input) != Size {
		return Nil
	}

	uuid := UUID{}
	copy(uuid[:], input)

	return uuid
}

@gaffneyc I'm thankful for the time you took to contribute this. Really solid improvement.