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

Reintroduce FileCheckpoints #3332

Merged

Conversation

hayley-jean
Copy link
Member

@hayley-jean hayley-jean commented Dec 1, 2021

Changed: Use file checkpoints on Linux and memory mapped checkpoints on Windows

jageall
jageall previously approved these changes Dec 1, 2021
@hayley-jean hayley-jean force-pushed the hayley-jean/re-introduce-file-checkpoints branch from 39ea50f to 8f5fe4e Compare December 1, 2021 16:04
jageall
jageall previously approved these changes Dec 1, 2021
@gregoryyoung
Copy link
Contributor

gregoryyoung commented Dec 1, 2021

Just as a bit of discussion ... There used to be memorymapped checkpoints a very long time ago (I implemented then).

If running with mmap checkpoints it was measurably more efficient to use a single checkpoint file that contained multiple checkpoints. If not clear writer/chaser/etc are different byte sections in the same mmap as opposed to using n mmaps as checkpoints are currently done with files. The difference was measurable.

update: 12/1 1315 with why this occurs ... the disk page size is going to be a minimum of 512 bytes. We can easily fit all of the checkpoints into this. As such you end up with one page flush for all checkpoints as opposed to a flush per checkpoint. While the ones other than writer are written less often this can be beneficial..

There were also some informal discussions about a slight change in how some of the checkpoints worked to instead move them into the footer of the current chunk (32 bytes) writer was the big one being looked at (the others are less important). This needed to be tested but was perceived to possibly be faster especially when also mmapping the tfchunks. And ... it could simplify things like the file structure/backup instruction/etc etc (no more ... checkpoint files! :-D)

The main reason for not moving things forward IIRC was "lets look again at the next major release" as it would change the file formats as well as things like backup procedures etc which we all know can be a bit of a pain to roll out :)

It might be a good time to revisit the idea.

condron
condron previously approved these changes Dec 1, 2021
Copy link
Contributor

@condron condron left a comment

Choose a reason for hiding this comment

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

Looks good

@thefringeninja thefringeninja added cherry-pick:master Cherry picks PR into master branch cherry-pick:release/oss-v21.10 Cherry picks PR into v21.10 release branch labels Dec 2, 2021
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

lets just hold on this a sec while we investigate the performance. for me the regular file checkpoint flushes at half the rate of the memory mapped one (on ntfs), seemingly because the regular one is updating the modified time a lot

going to check the behaviour on zfs

we might want to reconsider the default depending on what we find

- Reintroduce FileCheckpoint
- Use FileCheckpoints on Linux and MemoryMappedCheckpoints on Windows
@hayley-jean hayley-jean dismissed stale reviews from condron and jageall via 97b3e03 December 2, 2021 17:11
@hayley-jean hayley-jean force-pushed the hayley-jean/re-introduce-file-checkpoints branch from 8f5fe4e to 97b3e03 Compare December 2, 2021 17:11
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

Doing a simple write/flush/read loop on the checkpoints yielded the following.

We see the memorymapped version is quicker on windows but on linux they perform the same as each other, so choosing according to OS looks good, especially since this is the same behaviour as we had while it used to run on mono

USB PLATTER, NTFS
MemoryMappedFileCheckpoint did 200 flushes in 00:00:02.3359016. 86 flushes/second
RegularFileCheckpoint      did 200 flushes in 00:00:05.3217928. 38 flushes/second

NVME, NTFS
MemoryMappedFileCheckpoint did 10000 flushes in 00:00:03.1795682. 3,145 flushes/second
RegularFileCheckpoint      did 10000 flushes in 00:00:05.3865895. 1,856 flushes/second

aws zfs (gp2?) uncompressed
ath: /srv/eventstore/data/tim.chk
Flushes: 10000
MemoryMappedFileCheckpoint did 10000 flushes in 00:00:07.9647080. 1,256 flushes/second
RegularFileCheckpoint      did 10000 flushes in 00:00:08.4984878. 1,177 flushes/second

aws zfs gp3 uncompressed
Path: /srv/eventstore/data/tim.chk
Flushes: 10000
MemoryMappedFileCheckpoint did 10000 flushes in 00:00:08.5280754. 1,173 flushes/second
RegularFileCheckpoint      did 10000 flushes in 00:00:08.6633967. 1,154 flushes/second

aws zfs gp3 compressed
Path: /srv/eventstore/data/tim.chk
Flushes: 10000
MemoryMappedFileCheckpoint did 10000 flushes in 00:00:08.6426804. 1,157 flushes/second
RegularFileCheckpoint      did 10000 flushes in 00:00:08.4456759. 1,184 flushes/second

the checkpoints also performed the same as each other on ext4

@gregoryyoung
Copy link
Contributor

gregoryyoung commented Dec 5, 2021

Doing a simple write/flush/read loop on the checkpoints yielded the following.

We see the memorymapped version is quicker on windows but on linux they perform the same as each other, so choosing according to OS looks good, especially since this is the same behaviour as we had while it used to run on mono

USB PLATTER, NTFS
MemoryMappedFileCheckpoint did 200 flushes in 00:00:02.3359016. 86 flushes/second
RegularFileCheckpoint      did 200 flushes in 00:00:05.3217928. 38 flushes/second

NVME, NTFS
MemoryMappedFileCheckpoint did 10000 flushes in 00:00:03.1795682. 3,145 flushes/second
RegularFileCheckpoint      did 10000 flushes in 00:00:05.3865895. 1,856 flushes/second

aws zfs (gp2?) uncompressed
ath: /srv/eventstore/data/tim.chk
Flushes: 10000
MemoryMappedFileCheckpoint did 10000 flushes in 00:00:07.9647080. 1,256 flushes/second
RegularFileCheckpoint      did 10000 flushes in 00:00:08.4984878. 1,177 flushes/second

aws zfs gp3 uncompressed
Path: /srv/eventstore/data/tim.chk
Flushes: 10000
MemoryMappedFileCheckpoint did 10000 flushes in 00:00:08.5280754. 1,173 flushes/second
RegularFileCheckpoint      did 10000 flushes in 00:00:08.6633967. 1,154 flushes/second

aws zfs gp3 compressed
Path: /srv/eventstore/data/tim.chk
Flushes: 10000
MemoryMappedFileCheckpoint did 10000 flushes in 00:00:08.6426804. 1,157 flushes/second
RegularFileCheckpoint      did 10000 flushes in 00:00:08.4456759. 1,184 flushes/second

the checkpoints also performed the same as each other on ext4

There is also a DirectIOCheckpoint (perhaps named unbufferedcheckpoint) sitting around somewhere. It would be worth benchmarking this as well. I went through looking quickly but did not see it. Note: directio will not update file times as well.

This would give you the 3 major IO mechanisms within the current checkpoint logic as a benchmark before looking at possible changes.

If it has been lost (its somewhere in history) I can re-implement it in all of 15-30 minutes for testing most of the code is still there I just dont see the checkpoint directly. The underlying code, I see ... it just needs a wrapper/ability to choose.

@jageall
Copy link
Contributor

jageall commented Dec 6, 2021

@gregoryyoung our benchmarks show the direct IO option being the fastest, but there is a specific issue with msync on some linux file systems right now that we need to address quickly and as msync/fsync has shown to be comparable and prevents an up to 15 second completion delay , we will go with the file checkpoints (these are what used to run exclusively on mono anyway) to fix this and deal with directio separately. related issue: openzfs/zfs#12662

@jageall jageall merged commit 4bb476b into release/oss-v20.10 Dec 6, 2021
@jageall jageall deleted the hayley-jean/re-introduce-file-checkpoints branch December 6, 2021 12:37
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.

🚨 @jageall Failed to create cherry Pick PR due to error:

RequestError [HttpError]: Merge conflict
   at /home/runner/work/_actions/EventStore/Automations/master/cherry-pick-pr-for-label/node_modules/@octokit/request/dist-node/index.js:66:23
   at processTicksAndRejections (internal/process/task_queues.js:97:5) {
 status: 409,
 headers: {
   'access-control-allow-origin': '*',
   'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, Deprecation, Sunset',
   connection: 'close',
   'content-length': '110',
   'content-security-policy': "default-src 'none'",
   'content-type': 'application/json; charset=utf-8',
   date: 'Mon, 06 Dec 2021 12:38:03 GMT',
   'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
   server: 'GitHub.com',
   'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
   vary: 'Accept-Encoding, Accept, X-Requested-With',
   'x-content-type-options': 'nosniff',
   'x-frame-options': 'deny',
   'x-github-media-type': 'github.v3; format=json',
   'x-github-request-id': '07C9:7098:A7F074:176679A:61AE042A',
   'x-ratelimit-limit': '1000',
   'x-ratelimit-remaining': '990',
   'x-ratelimit-reset': '1638797880',
   'x-ratelimit-resource': 'core',
   'x-ratelimit-used': '10',
   'x-xss-protection': '0'
 },
 request: {
   method: 'POST',
   url: 'https://api.github.com/repos/EventStore/EventStore/merges',
   headers: {
     accept: 'application/vnd.github.v3+json',
     'user-agent': 'octokit-core.js/3.3.2 Node.js/12.22.7 (linux; x64)',
     authorization: 'token [REDACTED]',
     'content-type': 'application/json; charset=utf-8'
   },
   body: '{"base":"cherry-pick-cherry-pick/3332/hayley-jean/re-introduce-file-checkpoints-master-1f7877cd-c75f-4be9-aacb-4880c6f69dc9","commit_message":"Merge 97b3e037fe119c8fea8ecf1b457a1edff5ace037 into cherry-pick-cherry-pick/3332/hayley-jean/re-introduce-file-checkpoints-master-1f7877cd-c75f-4be9-aacb-4880c6f69dc9 [skip ci]\\n\\n\\nskip-checks: true\\n","head":"97b3e037fe119c8fea8ecf1b457a1edff5ace037"}',
   request: { agent: [Agent], hook: [Function: bound bound register] }
 },
 documentation_url: 'https://docs.github.com/rest/reference/repos#merge-a-branch'
}

🚨👉 Check https://github.com/EventStore/EventStore/actions/runs/1544623389

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.

🚨 @jageall Failed to create cherry Pick PR due to error:

RequestError [HttpError]: Merge conflict
   at /home/runner/work/_actions/EventStore/Automations/master/cherry-pick-pr-for-label/node_modules/@octokit/request/dist-node/index.js:66:23
   at processTicksAndRejections (internal/process/task_queues.js:97:5) {
 status: 409,
 headers: {
   'access-control-allow-origin': '*',
   'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, Deprecation, Sunset',
   connection: 'close',
   'content-length': '110',
   'content-security-policy': "default-src 'none'",
   'content-type': 'application/json; charset=utf-8',
   date: 'Mon, 06 Dec 2021 12:38:07 GMT',
   'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
   server: 'GitHub.com',
   'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
   vary: 'Accept-Encoding, Accept, X-Requested-With',
   'x-content-type-options': 'nosniff',
   'x-frame-options': 'deny',
   'x-github-media-type': 'github.v3; format=json',
   'x-github-request-id': '07C1:6975:123198E:22C02E1:61AE042F',
   'x-ratelimit-limit': '1000',
   'x-ratelimit-remaining': '978',
   'x-ratelimit-reset': '1638797880',
   'x-ratelimit-resource': 'core',
   'x-ratelimit-used': '22',
   'x-xss-protection': '0'
 },
 request: {
   method: 'POST',
   url: 'https://api.github.com/repos/EventStore/EventStore/merges',
   headers: {
     accept: 'application/vnd.github.v3+json',
     'user-agent': 'octokit-core.js/3.3.2 Node.js/12.22.7 (linux; x64)',
     authorization: 'token [REDACTED]',
     'content-type': 'application/json; charset=utf-8'
   },
   body: '{"base":"cherry-pick-cherry-pick/3332/hayley-jean/re-introduce-file-checkpoints-release/oss-v21.10-df595fb9-5a1d-42ae-b16e-2202dd30c455","commit_message":"Merge 97b3e037fe119c8fea8ecf1b457a1edff5ace037 into cherry-pick-cherry-pick/3332/hayley-jean/re-introduce-file-checkpoints-release/oss-v21.10-df595fb9-5a1d-42ae-b16e-2202dd30c455 [skip ci]\\n\\n\\nskip-checks: true\\n","head":"97b3e037fe119c8fea8ecf1b457a1edff5ace037"}',
   request: { agent: [Agent], hook: [Function: bound bound register] }
 },
 documentation_url: 'https://docs.github.com/rest/reference/repos#merge-a-branch'
}

🚨👉 Check https://github.com/EventStore/EventStore/actions/runs/1544623389

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 cherry-pick:release/oss-v21.10 Cherry picks PR into v21.10 release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants