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

Flush to database based on size instead of entries #5531

Closed
imsodin opened this Issue Feb 13, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@imsodin
Copy link
Member

commented Feb 13, 2019

This issue is in reaction to https://forum.syncthing.net/t/pretty-drastic-amount-of-writes-when-doing-initial-scan/12850 and a regression in #5441. Already since before that regression batch writes are flushed to db based on number of entries in the batch. That's okish, if entries are of the same size. However since #5441 that's no longer true, as full file infos are written in the same batch as e.g. blockmap entries. Before file infos were flushed after 64 batched items, blockmap entries after 1000, now it's 64 for both together.

I propose to use func (b *Batch) Dump() []byte to check the byte length of the batch and decide whether to flush based on that. Dump does not copy the data, so it should be efficient. Handwaving conversion of previous limits (64*fileinfo = 64 * (2000 blocks + more) = 64 * (8kB + 8kB) = 1MB) increasing a bit due to Moore gets me to 8MB limit. Any other (potentially more informed) propositions?

@imsodin imsodin added the bug label Feb 13, 2019

@imsodin imsodin self-assigned this Feb 13, 2019

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Sounds good, yet I'd validate that that is the case with Dump(), or batch could track the size as puts are done.

Also I'd halve the size to 4 just in case of rpi with a large number of folders.

@calmh

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@fingon

This comment has been minimized.

Copy link

commented Feb 14, 2019

I recommend time limit; e.g. flush every second. That would reduce LSM tree torture a lot when doing initial scanning (possibly some large byte limit additionally, but usually amount of changes you can accumulate per second is limited).

@imsodin

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

So I started to actually read a bit about how goleveldb/LSMs work (maybe I should do that first next time). Maybe I am explaining stuff that's obvious to you all, but it wasn't to me and is relevant to the change I outlined above:
Flushing a Batch does not mean flushing to disk. It's purpose is to accumulate multiple changes into one atomic change. When flushed they get written to a db stage in memory, which in turn gets flushed to disk when exceeding leveldb.Options.WriteBuffer. To improve disk performance we'd need to tweak leveldb options like mem buffer size and level base size and multipliers, not primarily think about when to flush batches. The main reason to choose when to flush batches should be keeping the committed data consistent, performance is secondary.
Basically, the short version of the above is: The proposed change will probably have a small impact on disk performance if at all, however it's still the right thing to do in my opinion.

Maybe take a peek at that Dump() to see what it does; if it just returns an existing slice that’s great.

Already checked that, it's exactly what it does.

I recommend time limit; e.g. flush every second. That would reduce LSM tree torture a lot when doing initial scanning (possibly some large byte limit additionally, but usually amount of changes you can accumulate per second is limited).

I doubt that limit would hit In practice: We already batch updates passed to Syncthing's db package, which will be flushed at the latest once fully processed - I wouldn't expect processing one batch to take more than a second.

@fingon

This comment has been minimized.

Copy link

commented Feb 14, 2019

The intent was to limit number of batches to one per second. Currently there are .. N .. per second and losing one second of work in case of crash or reboot is not particularly daunting.

imsodin added a commit to imsodin/syncthing that referenced this issue Feb 14, 2019

lib/db: Flush batch based on size and refactor (fixes syncthing#5531)
Flush the batch when exceeding a certain size, instead of when reaching a number
of batched operations.
Move batch to lowlevel to be able to use it in NamespacedKV.
Increase the leveldb memory buffer from 4 to 16 MiB.
@imsodin

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Yep, that would have to happen on the model side of things, were we currently batch at a fixed limit of 1000 items per batch (or earlier based on size). That's coming from sending indexes, which is probably not ideal for committing to database.

Does anyone know the difference of batch writes to transactions for leveldb?
They look very similar to me and the goleveldb documentation doesn't help me (https://godoc.org/github.com/syndtr/goleveldb/leveldb) and I don't find anything for leveldb (as a matter of fact, I don't find any documentation for that). My current understanding from this issue leading to the creation of transactions in goleveldb (syndtr/goleveldb#129) is, that transactions somehow "bypass a step" (no idea what the "step" actually is).

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Ping @syndtr for advice perhaps

@calmh calmh added this to the v1.1.1 milestone Feb 25, 2019

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.