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

Add fsyncing bloomfilter #3425

Merged

Conversation

timothycoleman
Copy link
Contributor

@timothycoleman timothycoleman commented Feb 14, 2022

Fixed: Improved Stream Existence Filter flushes

For #3423

While the stream existence filter is flushing (msync) there appears to be a small chance that ordinary writes can be delayed several seconds (highest observed was 25s on ZFS, GCP).

This may be ZFS specific, possibly related (though not identical) to openzfs/zfs#12662 for which reason we recently switched away from memory mapped checkpoints on zfs (#3332)

This PR adds a fsync (as opposed to msync) strategy for the persistent bloom filter. By avoiding the memory mapped file (1) we are no longer calling msync, avoiding the problem if it is ZFS related and (2) we take control of flushing directly so that we can control the rate and ensure it has a low impact on other disk operations regardless

@timothycoleman timothycoleman force-pushed the timothycoleman/fsyncing-bloomfilter branch from 95c5b03 to abc7823 Compare February 14, 2022 10:57
@tambeau tambeau requested review from jageall and removed request for jageall February 14, 2022 14:24
@gregoryyoung
Copy link
Contributor

@timothycoleman are you adding an unbuffered stream version of this as well?

If so is there a significant difference in how the caching needs to work?

@timothycoleman
Copy link
Contributor Author

@gregoryyoung not for the minute, but once we're on net6 the new random file access stuff in there could have a bearing i think

@gregoryyoung
Copy link
Contributor

gregoryyoung commented Feb 16, 2022

There is the current unbufferedstream in our codebase (it exists for a reason :-D), do you want me to spend a bit tomorrow or Friday to see if I can quickly work it in as a spike?

https://github.com/EventStore/EventStore/blob/77d18b6b6b660f68e09630f80ca9a7e9ee695b79/src/EventStore.Core/TransactionLog/Unbuffered/UnbufferedFileStream.cs

Doing via it will also be immediately cross platform which other things may be less than for ... some ... time ...

@timothycoleman
Copy link
Contributor Author

This is holding up a patch release at the moment so we'll probably take the shortest path to getting that out (i.e. the FileStream at least for the short term) but that said, the UnbufferedFileStream stuff looks interesting and like it might be a pretty easy fit. Feel free to take a look of course, but no promises on when I might get back to it. I'm back on to scavenging after this

@gregoryyoung
Copy link
Contributor

gregoryyoung commented Feb 16, 2022

Get the patch out and lets revisit.

I wish we could say :

github add comment eventstore pull3425 72 hours "wake up"

:)

Oh ... and drop me a note re: scavenging. I originally wrote most of that code and know it quite well.

// This class is responsible for maintaining and validating a 4 byte hash at the end
// of the cache line.
public class BloomFilterIntegrity {
public const int CacheLineSize = 64;
public const int HashSize = 4;
//qq this is the flush unit, discuss with james whether it should be configurable and what the default should be.
public const int PageSize = 8 * 1024;
Copy link
Member

@shaan1337 shaan1337 Feb 17, 2022

Choose a reason for hiding this comment

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

it could also be looked up at runtime with sysconf(_SC_PAGESIZE) on linux and GetSystemInfo() on windows with a fallback to the usual default (4096) if there was any problem looking it up

Copy link
Member

Choose a reason for hiding this comment

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

if it is configurable, the value 0 could be used for auto-configuration (similar to how it is for other auto-configurable options)

Copy link
Member

Choose a reason for hiding this comment

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

note: if it is configurable, it should be a multiple of the cache line size (64) and disk block size (4096)

- takes more precise control over flushing
- tries to share the IOPS nicely with other writes
- avoids possible complications around using msync with zfs
@timothycoleman timothycoleman force-pushed the timothycoleman/fsyncing-bloomfilter branch from 74f7143 to 10b7c43 Compare February 18, 2022 13:32
@timothycoleman timothycoleman marked this pull request as ready for review February 18, 2022 13:32
@timothycoleman timothycoleman added the cherry-pick:master Cherry picks PR into master branch label Feb 18, 2022
Copy link
Contributor

@gregoryyoung gregoryyoung left a comment

Choose a reason for hiding this comment

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

With the BloomFilterAccessor which is using off heap memory has it been considered to use buffers from BufferManager?

This would put all of that memory usage through the same known mechanism as other things are using.

This would have both some pros and cons associated and is not by any mean a "this is a great idea that is a definite improvement" type thing, more I am wondering if it was considered.

jageall and others added 4 commits February 21, 2022 17:35
…eamPersistence.cs

Co-authored-by: Shaan Nobee <sniper111@gmail.com>
…eamPersistence.cs

Co-authored-by: Shaan Nobee <sniper111@gmail.com>
…lterAccessor.cs

Co-authored-by: Shaan Nobee <sniper111@gmail.com>
jageall
jageall previously approved these changes Feb 21, 2022
@timothycoleman
Copy link
Contributor Author

@gregoryyoung I hadn’t considered the buffer manager, I’ll take a look to find out more about it, thanks for the pointer

the intptr/long thing is because allochglobal takes an intptr and addmemorypressure takes a long. Although, I wonder if we really need both constructors. I’ll check when I’m in the office

@timothycoleman timothycoleman merged commit 4898da1 into release/oss-v21.10 Feb 22, 2022
@timothycoleman timothycoleman deleted the timothycoleman/fsyncing-bloomfilter branch February 22, 2022 12:59
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

@timothycoleman 👉 Created pull request targeting master: #3438

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick:master Cherry picks PR into master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants