Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nuid #360

Merged
merged 30 commits into from
Jan 28, 2020
Merged

Nuid #360

merged 30 commits into from
Jan 28, 2020

Conversation

jasper-d
Copy link
Contributor

@jasper-d jasper-d commented Jan 26, 2020

Notable changes:

  • Created a new Nuid implementation (internal for now) because adapting the existing one w/o introducing breaking changes was not desirable in terms of performance and idiomatic (.NET-ish) interface
  • Marked the old implementation as obsolete so it can be removed in a future release
  • Removed the different inbox algorithms for old and new requests styles (not sure why the distinction was made)
  • Opted to use base-64 instead of base-62 to avoid modulo bias (cf. Modulo bias in prefix generation nuid#15) and gain some performance
  • Added a micro benchmark project (uses BenchmarkDotNet) to ensure performance is acceptable (see results below). All performance optimizations are based on evidence provided by benchmark runs.
  • Made NATS.Client internals visible to UnitTests and MicroBenchmarks

Let me know if you have questions/change requests

Edit:

Haven't changed the token generation yet (nats-io/nats.net#351 (comment)), would do so in a seperate PR

Issue for reference: #351

Benchmark Results

NextNuid is in this PR, NUIDNext is the existing (unused) NUID implementation, OldNewInbox is the currently used implementation (https://github.com/jasper-d/nats.net/blob/bd2abf39df3b7c7c06d12ec9a0069e066650f504/src/Benchmarks/MicroBenchmarks/Program.cs).

Windows

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18362
Intel Core i7-4712MQ CPU 2.30GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.1.101
  [Host]     : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT
  Job-JHNFEY : .NET Framework 4.8 (4.8.4075.0), X64 RyuJIT
  Job-LNVMVJ : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT


Method Runtime Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
NextNuid .NET 4.6.2 75.57 ns 0.039 ns 0.035 ns 0.17 0.0459 - - 144 B
NUIDNext .NET 4.6.2 454.71 ns 1.150 ns 1.076 ns 1.00 0.0458 - - 144 B
OldNewInbox .NET 4.6.2 789.66 ns 0.695 ns 0.616 ns 1.74 0.1040 - - 329 B
NextNuid .NET Core 3.1 81.45 ns 0.368 ns 0.344 ns 0.19 0.0458 - - 144 B
NUIDNext .NET Core 3.1 436.05 ns 0.296 ns 0.277 ns 1.00 0.0458 - - 144 B
OldNewInbox .NET Core 3.1 586.60 ns 0.713 ns 0.667 ns 1.35 0.0706 - - 224 B

Linux

BenchmarkDotNet=v0.12.0, OS=fedora 31
Intel Core i7-3612QM CPU 2.10GHz (Ivy Bridge), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.1.101
  [Host]     : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT
  Job-AEFKXL : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT

Runtime=.NET Core 3.1
Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
NextNuid 116.7 ns 0.35 ns 0.33 ns 0.19 0.0458 - - 144 B
NUIDNext 599.7 ns 0.59 ns 0.52 ns 1.00 0.0458 - - 144 B
OldNewInbox 761.4 ns 0.62 ns 0.58 ns 1.27 0.0706 - - 224 B

nuid.go for reference (the figures differ quite a lot between runs, but these are the lowest numbers I got):

[jd@linux]~/sources/nuid.origin%  go test -bench=.

goos: linux
goarch: amd64
BenchmarkNUIDSpeed-8            12650074                91.9 ns/op            32 B/op          1 allocs/op
BenchmarkGlobalNUIDSpeed-8      11843164               101 ns/op              32 B/op          1 allocs/op
PASS
ok      _/home/jd/sources/nuid.origin   6.192s

@ColinSullivan1
Copy link
Member

We dove right in making comments before saying thank you for the PR. Just wanted to leave a note saying that this is really good and highly appreciated!

@jasper-d
Copy link
Contributor Author

Thank you, much appreciated. Comments are fair, so it's all good. :)

@jasper-d
Copy link
Contributor Author

Sorry, added another commit. Changed most uses of signed integers to unsigned ones. I think this actually improves the code since non-negative integers are expected in multiple places. Also removed a suspicious cast to Int32.

Copy link
Member

@ColinSullivan1 ColinSullivan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

Copy link
Member

@ColinSullivan1 ColinSullivan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@ColinSullivan1 ColinSullivan1 merged commit ee6f739 into nats-io:master Jan 28, 2020
@watfordgnf
Copy link
Collaborator

For posterity, here are the benchmarks on my workstation:

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.17763.973 (1809/October2018Update/Redstone5)
Intel Xeon Silver 4114 CPU 2.20GHz, 2 CPU, 20 logical and 20 physical cores
  [Host]     : .NET Framework 4.8 (4.8.4075.0), X64 RyuJIT
  Job-OZBAHI : .NET Framework 4.8 (4.8.4075.0), X64 RyuJIT
  Job-MNUBEA : .NET Core 3.1.0 (CoreCLR 4.700.19.56402, CoreFX 4.700.19.56404), X64 RyuJIT
Method Runtime Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
NextNuid .NET 4.6.2 105.72 ns 0.392 ns 0.367 ns 0.23 0.0229 - - 144 B
NUIDNext .NET 4.6.2 462.81 ns 1.806 ns 1.690 ns 1.00 0.0224 - - 144 B
NextNuid .NET Core 3.1 92.28 ns 0.558 ns 0.466 ns 0.21 0.0200 - - 144 B
NUIDNext .NET Core 3.1 449.42 ns 1.683 ns 1.574 ns 1.00 0.0200 - - 144 B

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants