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

feat: fs-repo-11-to-12 special flatfs handling #152

Merged
merged 8 commits into from
Feb 17, 2022

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented Feb 15, 2022

Trying to add some special casing fast-path code for flatfs.

Some things that are currently missing that may be problematic are:

  • We're not using the datastore which means things like disk usage could technically end up out of sync, doesn't seem relevant though since we're not adding or removing data
  • Reversions do not leave behind data copies (e.g. a CIDv0 and a CIDv1) which effects both the disk usage and more importantly the discoverability of data. Good news here is that we could use the slow code path for reversion and things should be fine (first let's see if CI catches the bug)
    • CI did catch the bug 🎉, and using the slow path for the reversion seems fine. I hope/suspect reversion of this migration will be very uncommon anyway

@aschmahmann aschmahmann changed the title [WIP] feat: fs-repo-11-to-12 special flatfs handling feat: fs-repo-11-to-12 special flatfs handling Feb 15, 2022
Copy link
Contributor Author

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Some preliminary testing on my local machine indicates this is much faster, so it'd be great if we could land this.

fs-repo-11-to-12/migration/setup.go Show resolved Hide resolved
fs-repo-11-to-12/migration/swapper.go Show resolved Hide resolved
fs-repo-11-to-12/migration/swapper.go Outdated Show resolved Hide resolved
@aschmahmann
Copy link
Contributor Author

Did some local benchmarking:

On my HDD (running on Windows)

  • 190k FlatFS moves in ~10minutes (Disk usage showed 100% on that drive which is only being used for this migration)
  • 16k regular moves in ~11 min (Disk usage was ~90% on the drive)

I suspect SSDs which have better random access will perform better here. There's probably also clever ways to group up renames to maximize mechanical sympathies, but what we have now seems like a pretty good improvement.


Looking at this migration running on my HDD (using a single worker) I'm actually a little concerned that users with lots of data might be annoyed at the disk usage during the migration and might ask us to tune it down. If users have non-huge repos and are using SSDs (e.g. most people on laptops and desktops) they should be fine though. WDYT?

@hsanjuan
Copy link
Contributor

  • first let's see if CI catches the bug)

:-) I am very proud of my tests

Copy link
Contributor

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

This looks ok to me.

We should test it.

My only question is whether this should have a flag to enable (disabled by default), or to disable (enabled by default).

I guess the main danger is that this breaks on some weird mount configuration or something, but seems to take these things into account.

Copy link

@guseggert guseggert left a comment

Choose a reason for hiding this comment

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

TBH, pushing this in at the last second makes me slightly uncomfortable because it hasn't been exercised by users w/ the RC.

The users who benefit from this are the ones with default datastore configs, are there a lot of such users who have huge repos that would benefit from this optimization? What's the "typical" speedup that a typical user would see?

fs-repo-11-to-12/migration/setup.go Outdated Show resolved Hide resolved
fs-repo-11-to-12/migration/swapper.go Show resolved Hide resolved
Also log total swapped in flatfs specific migration
@aschmahmann
Copy link
Contributor Author

The users who benefit from this are the ones with default datastore configs, are there a lot of such users who have huge repos that would benefit from this optimization? What's the "typical" speedup that a typical user would see?

Hard to say. I think most people have the default configs and some people with really big repos have started coming back to FlatFS after trying out Badger and running into problems, so this is a bunch of users.

The larger repo I was testing on locally (although I didn't finish the full migration) was about 80GB with 1.2M CIDv1s and I was seeing a 10x speedup. I'd suspect typical users might see even bigger speedups if they have SSDs, but haven't directly tested that yet.

TBH, pushing this in at the last second makes me slightly uncomfortable because it hasn't been exercised by users w/ the RC.

True and me too, although I'm not sure what our exercise rate on v0.12.0-rc1 has been despite how long it's been out for. In theory we can just do RC2 and wait a week or so and do the final release, but I'm not sure how much value that brings us.

…ngleton anyway. close datastore after use in migration tests
@aschmahmann aschmahmann mentioned this pull request Feb 16, 2022
59 tasks
@aschmahmann
Copy link
Contributor Author

Did some local testing of env vars just to double check (Go tests aren't too happy about dealing with them) and everything looks good so we're going to hit some buttons and do a release 🎉

@aschmahmann aschmahmann merged commit 3dc218e into master Feb 17, 2022
@aschmahmann aschmahmann deleted the feat/fs-repo-11-to-12-flatfs branch February 17, 2022 22:29
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.

3 participants