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 MessageTemplateCache performance #883

Merged
merged 3 commits into from Oct 20, 2016

Conversation

Projects
None yet
3 participants
@skomis-mm
Copy link

skomis-mm commented Oct 19, 2016

  • fixes #823 for all except netstandard10-12
  • added perf tests for normal (cached) and leaking scenarios: netcoreapp10, net452

based on old forgotten Hashtable. According to docs it is thread-safe for multiple readers (no enumerations) and single writer. So looked like a good fit for this specific scenario with tradeoff on locking only writers in exchange on a cheap Count property.

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Oct 20, 2016

That's fascinating, I completely overlooked Hashtable. Looks like great results!

@nblumhardt nblumhardt merged commit ba795ac into serilog:dev Oct 20, 2016

4 checks passed

codecov/patch 100% of diff hit (target 67.62%)
Details
codecov/project 67.62% (+0.00%) compared to 0c8f646
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dotnetjunkie

This comment has been minimized.

Copy link

dotnetjunkie commented Oct 20, 2016

Holy crap. That HashTable got some really complex lock free code using spinwaits and Thread.Sleep. Must be written by @joeduffy.

@skomis-mm skomis-mm deleted the skomis-mm:msgtplcache branch Oct 20, 2016

@dotnetjunkie

This comment has been minimized.

Copy link

dotnetjunkie commented Jan 11, 2017

Warning, @davkean just warned here about HashTable not being thread-safe, not even with hread-safe for multiple readers and a single writer.

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Jan 11, 2017

Thanks @dotnetjunkie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.