util/uuid: fork gofrs, generate with math/rand instead of crypto/rand

[ajwerner]

Fixes #30236
Copies the source from github.com/gofrs/uuid into util/uuid at 7077aa61129615a0d7f45c49101cd011ab221c27.
Modifies the copyright notice to refer to the license at licenses/MIT-gofrs.txt.
Renames the spurious Bytes method to bytes.
Adds the ability to create a Gen with a random byte Reader.
Add FastMakeV4() which uses a generator which draws from a math/rand.Rand.
Use FastMakeV4 in roachpb.MakeTransaction.

Release note (performance improvement): Use faster randomness source to generate transaction IDs.

[cockroach-teamcity]

This change is 

[bdarnell]

Sets the DefaultGenerator to one based on math/rand.Rand.
Use secure crypto/rand in sql uuid builtins.

This should go the other way: use crypto/rand by default, and only switch to math/rand where we know performance is critical and we don't need stronger randomness (just transaction ids?)

[knz]

1. this:

This should go the other way: use crypto/rand by default, and only switch to math/rand where we know performance is critical and we don't need stronger randomness (just transaction ids?)

What Ben says.

2. in general a commit message / PR description should contain the following, in this order:

   1. a summary of the current situation
   2. a short explanation of why the current situation is problematic
   3. what the commit is doing to change the current situation
   4. what the new situation is as a result

When the change is aiming to improve performance:

• there must be a benchmark that validates the performance change:

  • either the benchmark exists already, in which case the commit message should describe which benchmark was used to validate
  • or the commit should contain new benchmark code

• the commit message should include a copy of the benchmark before/after comparison, eg via the benchstat tool.

[knz]

Also looking at the linked issue #30236 there is only mention of accelerating txn ID generation. So as a word of wisdom, to be reused generally when thinking of changing the "provider" of some of the util packages:

• audit every caller of the util package, and make a list of every context where the caller is used -- answer the question "is there anywhere else that's possibly impacted besides the specific problem I'm looking at?"

• share this list in the commit message

• for each of them make your own assessment of how the change is going to impact.

  • if the change is only performance-oriented, assess using a benchmark
  • if the change is potentially correctness-related (as is the case here), make an assessment of how the new behavior differs from the expectations from specs
    • in your case here, for the txn ID case this assessment was made by Nathan in the linked issue. You can copy-paste his argument in your commit message, or make a summary thereof
    • in your case here, for the SQL usage you need to assess yourself
    • there may be other callers I'm not seeing yet (you'd need to assess yourself too)
  • share the assessments in the commit message / PR description

[ajwerner]

Fixed the comment with regards to the default. I'll go through the other callers and see if they occur on hot paths, but for now updated the diff to just touch the transaction ID.

Reviewable status: complete! 0 of 0 LGTMs obtained

[benesch]

but please wait for someone else to sign off as well!

Reviewed 60 of 60 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained

[nvanbenschoten]

although I'd like to see a benchmark demonstrating the benefit that we observe from this change.

Reviewed 18 of 60 files at r1.
Reviewable status: complete! 2 of 0 LGTMs obtained

[knz]

Huh wait

There is a huge chunk of new code in the pkg/util package. There is zero explanation for this code in the commit message. This contrast between the two things does not sit well with me.

See my comments above about what a commit message should state.

See also the comment above by Ben - how did you actually address the requirement to provide different UUID sources for SQL? Did you do so actually? (It's not so clear to me how/where this is happening. The commit message should guide me.)

It seems you copied code from 3rd party sources into the pkg/util directory. Code imports, together with a highlight of the license that allowed you to import this code, should be called out in the commit message.

Also repeat of my invitation to provide benchmark comparisons.

Reviewed 3 of 60 files at r1.
Reviewable status: complete! 2 of 0 LGTMs obtained

[benesch]

Are we looking at different commit messages, @knz? This is the one I'm looking at:

Copies the source from github.com/gofrs/uuid into util/uuid at 7077aa61129615a0d7f45c49101cd011ab221c27.
Remove github.com/satori/go.uuid dep
Modifies the copyright notice to refer to the license at licenses/MIT-gofrs.txt.
Renames the spurious Bytes method to bytes.
Adds the ability to create a Gen with a random byte Reader.
Add FastMakeV4() which uses a generator which draws from a math/rand.Rand.
Use FastMakeV4 in roachpb.MakeTransaction.

I suppose there could be more rationale but it seems to speak to the questions you've asked.

Here's how I would phrase the commit message, @ajwerner, just FYI. I'm also fine with what you already have!

util/uuid: support generation of fast but non-cryptographically secure UUIDs

Replace our UUID library, github.com/satori/go.uuid, which was unmaintained and did not provide a facility for customizing the source of randomness. The new library is based upon the MIT-licensed github.com/gofrs/uuid library, which is better maintained and allows customizing the source of randomness. Because the code is so simple, copy and paste it wholesale rather than vendoring it. This will ease future customizations.

The new library exposes a fast UUID generation function, FastUUIDV4, that uses a non-cryptographically secure function for generating UUIDs. For now, only transaction ID generation uses the new fast generation function, as transaction IDs are only used internally and therefore do not need a cryptographically-secure source of randomness. Other calls to UUID generation, like the SQL builtins, continue to use a cryptographically-secure source of randomness.

Reviewable status: complete! 2 of 0 LGTMs obtained

[knz]

Yes indeed I didn't really read the commit message because it was all mashed up together and appeared to me initially as a mere list of steps that were taken without visibility on neither motivation nor end result. However I apologise for misreading and my resulting wrong impression. The information I was seeking was indeed present and I would not have commissioned if I had paid more attention. My sincerest excuses. That said a note about the multiple sources and which caller uses which source would be welcome. And a benchmark 😺 With love,

…

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[nvanbenschoten]

Part of the expected improvement from #30236 is Mac-specific. I know you don't have a Mac laptop, so I took this for a quick spin to verify the effect. Sure enough, the effect is immediately clear on Mutex profiles when running kv95 --concurrency=64 on my laptop.

before:

after:

The baseline throughput difference on the benchmark was somewhere around .5%, but I didn't run enough trials for the results to be statistically significant:

name  old ops/sec  new ops/sec  delta
KV95   7.95k ± 1%   8.00k ± 1%   ~     (p=0.200 n=4+4)

[ajwerner]

From micro benchmarks on my Linux laptop I see the below compelling win. I'm running kv benchmarks on roachprod now.

name          time/op
FastMakeV4-8  65.5ns ±14%
MakeV4-8       646ns ± 0%

Also added the benchmark to the code and updated the commit message along the lines of @benesch's suggestion.

[ajwerner]

Didn't see any statistically significant improvement on 32 core machines but got the below results on 3 64 core machines running kv with 95% reads, with concurrency set to 512:

name       old ops/s   new ops/s   delta
KV95     9.36k ± 1%  9.52k ± 1%  +1.81%  (p=0.016 n=5+5)

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)