bolt icon indicating copy to clipboard operation
bolt copied to clipboard

Suggested edit to concurrency / mmap locking behavior description in README.md

Open dtromb opened this issue 8 years ago • 8 comments

I found the following text in the README.md a little bit confusing, and it ended up costing me a bunch of time / wasted code consequently:

Read-only transactions and read-write transactions should not depend on one another 
and generally shouldn't be opened simultaneously in the same goroutine. This can cause a
deadlock as the read-write transaction needs to periodically re-map the data file but it cannot 
do so while a read-only transaction is open.

In particular, a not-careful reading (like the one I gave it) might imply there are no locking problems whatsoever with concurrent read/read-write transactions so long as they are opened in different goroutines.

Perhaps it should say something like "under some circumstances (when the database memory map needs to grow), read/write Commit() will block until all read-only transactions in every goroutine are closed. In particular, care should be taken to not try to commit a read-write transaction before closing all reads in a single goroutine"?

dtromb avatar Jan 16 '16 17:01 dtromb

(Also, I'm speaking from a place of relative ignorance as I haven't gotten too deep into the bolt source yet, but it would be really nice if this sort of locking up on r/w Commit() didn't happen at all - is it somehow conceivable that this might be possible? Perhaps by using copy-on-write paging or something like that? I've never encountered this with other k/v stores...)

dtromb avatar Jan 16 '16 17:01 dtromb

@benbjohnson Just thinking about this now, is there a reason why we can only have one mapping of the database at a time? I know 32bit arches would have issues, but 64bit should have tons of VM space. Would a PR lifting this limitation but using multiple memory maps be considered? I had some ideas for how it could be arranged, though no guarantees on when that might turn into code.

I'd understand if you consider that out of scope, since you want boltdb to be a minimal KV and that would be too complicated. I just don't want to put pen to keyboard if that is the case.

MJDSys avatar Jan 16 '16 18:01 MJDSys

With #453 and #472 merged in, you can set InitialMmapSize to a high number (e.g. 1TB) and it'll only mmap once. Then you won't have an issue with transactions blocking other transactions. That option needs to be added to the README.

benbjohnson avatar Jan 17 '16 17:01 benbjohnson

Ah, that might work. This would only be possible on a 64-bit system, however, correct? Are there any downsides in performance characteristics are other concerns to be aware of mmap'ing a huge chunk of process space like that?

dtromb avatar Jan 18 '16 01:01 dtromb

Yes, only on a 64-bit system. However, you wouldn't be able to mmap it in multiple smaller chunks on a 32-bit system since you would run out of virtual space.

I don't know of any perf downsides with that approach. That's the approach that LMDB suggests.

benbjohnson avatar Jan 18 '16 16:01 benbjohnson

One thing I did come across is that on recent versions of Linux, the documentation claims that adding the MAP_HUGETLB flag to the mmap flags may be more performant for large mappings (it seems it encodes addresses differently internally, that way). I couldn't see a difference in my test cases, but if if it does make a difference perhaps it should be defaulted in if the initial mmap size is larger than some threshold value...

dtromb avatar Jan 18 '16 19:01 dtromb

I'd be up for adding it to the documentation but I'd like to avoid surprises when users bump from one size to another. There's a DB.MmapFlags field where users can add additional flags if they'd like (e.g. MAP_POPULATE) but many of those are dependent on use case.

benbjohnson avatar Jan 18 '16 20:01 benbjohnson

Makes sense. (I also just noticed it fails with EINVAL on kernel versions that don't support it!)

Looks like my original problem was completely resolved by #472 (thanks!) and setting initial map size, will leave this open for the sake of that doc change though.

dtromb avatar Jan 18 '16 21:01 dtromb