[WIP] Remove mmap usage in SPVBlockStore

[oscarguindzberg]

There has been a "TODO" for years to remove mmap usage in SPVBlockStore.

I replaced it by direct usage of RandomAccessFile

I am motivated to implement this because I found #1477 fix does not work on windows.

[schildbach]

Thanks for the effort! Do you have an idea if that change might affect performance?

[schildbach]

Ah, and the format is stays the same, right?

[schildbach]

I think it's a bit unfortunate that these two methods duplicate the above two methods. In theory, we could implement the new methods like this:

FileChannel channel = randomAccessFile.getChannel();
ByteBuffer buffer = ByteBuffer.allocate(CHAIN_WORK_BYTES);
deserializeCompact(params, buffer);

Of course I don't know about the performance implications and wether we run into specific problems in Windows again. Have you considered this?

[oscarguindzberg]

I added a commit with your suggestion "Avoid duplicating StoredBlock's serializeCompact() and deserializeCompact".
I will squash once we finish the discussion and testing.
I don't think this deduplication commit brings up the windows problem again, since the windows problem is just with mmaped files not with the uses of Buffers or FileChannels.
About performance, this deduplication commit does not improve/deteriorate performance. I will address your performance question in a broader PR comment.

[oscarguindzberg]

@schildbach
The block store file format is not changed.

About performance:
I guess this change will, in fact, deteriorate i/o performance (See https://en.wikipedia.org/wiki/Memory-mapped_file). I don't know how much worse it will be, we can test it but I guess it will depend on the use case.
Mike suggested to eventually remove the use of mmaped files back in 2015 07d85f2 but I don't know what other problems they cause on top of this windows specific problem on releasing the file control.

Since this will require lots of test and might be controversial, I think it would be better as a first step to make the change optional/just for windows. A couple of options:

• Overload constructors to add a boolean useMmapedFile parameter.
• Add some if (System.getProperty("os.name").toLowerCase().contains("win"))
• Create a public class NotMmappedFileSPVBlockstore extends SPVBlockstore

[schildbach]

For Android, Mike had this to say about mmap: "on Android each get() call is actually a full-blown JNI method under the hood, meaning it's unbelievably slow". So from my Android perspective, I'd probably be ok to just change the implementation to RandomAccessFile and be done with it. On Windows, RandomAccessFile seems to be the only available option anyway. So only Linux and MacOS are left to decide.

Yes, testing needs to be done. But we're entering a stabilization phase anyway.

[oscarguindzberg]

@schildbach What would be the next step with this PR?

About unit tests: they pass on my computer. Travis unit tests execution fails but I can't find out why.

[schildbach]

I'd prefer to keep only one implementation: either mmap or RandomAccessFile. I'm not afraid about testing, as we're about to enter the stabilization phase for 0.15. I leave it up to you wether you think we should try it for 0.15.

[oscarguindzberg]

A similar bugfix is being tested on the Bisq project bisq-network/bisq#2403 . Let's see what happens there before merging the PR.

[schildbach]

If you rebase this to current master, it should be picked up by the new performance test.

[ManfredKarrer]

I did some basic runtime tests with the Bisq app and it seems the performacne degradation at initial download is acceptable. I still would prefer to only apply it for Windows not for all OS. See bisq-network/bisq#2403 (comment)

[schildbach]

Ok, then I suggest a more straightforward name… SPVRandomAccessFileBlockStore?

[oscarguindzberg]

I rebased from master.

I ran SPVBlockStoreTest.performanceTest() on a mac
old mmapped-version takes 0.6 seconds
new non-mmapped-version takes 5.24 seconds.

This is around 10x performance loss on writing.

I will think more about this and post another comment.

[oscarguindzberg]

So...
Travis build fails because SPVBlockStoreTest.performanceTest() takes 2.2 seconds.
About performance:

• We can assume 10x performance loss on writing.
• The question is whether that matters or not. The blockchain is written a lot during chain sync. Chain sync might be huge for:
  • new wallets.
  • wallets being re-opened after a while.
  • restore from seed.
• If the wallet is connecting to a remote peer, p2p delays might be orders of magnitude bigger than the performance loss of not using mmapped file. But syncing to a local bitcoind is probably a common use case on several projects.
• We still no did test on performance when reading yet. There is a software cache on SPVBlockStore so it should prevent going to the file in many cases.

On a side note...
All this started because restore from seed fails on windows.
I started working on a different approach to solve that problem.
#1702
I am not sure what approach is better to move on with.

[oscarguindzberg]

I close this PR and suggest to merge #1702 instead.