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

Improve speed of GetRegionFromEndpoint with caching #406

Merged
merged 2 commits into from
Jun 1, 2020

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented May 1, 2020

I was pre-signing around 3,000 URLs in a single document and noticed a significant delay. I tracked it down to the GetRegionFromEndpoint.

Screenshot - 2020-05-01_00-10-11

I added a benchmarking project and eventually settled on caching as the fastest solution. This is still an improvement on #394 with more than double the speed of that significant improvement:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.778 (1909/November2018Update/19H2)
Intel Core i7-6700HQ CPU 2.60GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.1.201
  [Host]     : .NET Core 2.1.17 (CoreCLR 4.6.28619.01, CoreFX 4.6.28619.01), X64 RyuJIT
  DefaultJob : .NET Core 2.1.17 (CoreCLR 4.6.28619.01, CoreFX 4.6.28619.01), X64 RyuJIT

Method Mean Error StdDev
OriginalAws 1,068,177.22 ns 16,354.928 ns 14,498.218 ns
OriginalNonAws 150,811.90 ns 2,248.922 ns 2,103.643 ns
OriginalNonAwsPort 3,838,383.62 ns 36,755.305 ns 32,582.620 ns
PreCompiledAws 1,934.85 ns 19.688 ns 19.336 ns
PreCompiledNonAws 313.57 ns 4.027 ns 3.767 ns
PreCompiledNonAwsPort 200.38 ns 1.670 ns 1.481 ns
CurrentAws 82.25 ns 1.720 ns 2.048 ns
CurrentNonAws 60.71 ns 0.465 ns 0.412 ns
CurrentNonAwsPort 74.68 ns 0.585 ns 0.519 ns

@ngbrown
Copy link
Contributor Author

ngbrown commented May 18, 2020

@geluk, as the author of #394, you might be interested in this improvement.

@geluk
Copy link
Contributor

geluk commented May 18, 2020

That's a nice further gain in performance indeed. I'll add on to this that if you provide the region name when instantiating the MinioClient, the GetRegionFromEndpoint method is not called at all, which could be helpful if you're really looking to get as much performance out of it as possible, as it avoids any risk of lock contention in the concurrent dictionary.

@ngbrown
Copy link
Contributor Author

ngbrown commented May 19, 2020

@geluk. Good point. The client didn't really encourage the passing that in, especially with how the minio server doesn't display if/what the default region is.

ConcurrentDictionary does have a have a lock-free read path.

Copy link
Contributor

@Praveenrajmani Praveenrajmani left a comment

Choose a reason for hiding this comment

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

LGTM

@harshavardhana harshavardhana merged commit b74eb75 into minio:master Jun 1, 2020
@ngbrown ngbrown deleted the benchmark-regions branch June 1, 2020 19:10
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.

5 participants