search

peter-lawrey / Java-Chronicle

Java Indexed Record Chronicle
1.22k stars 193 forks source link

Chronicle without list of buffers #18

Closed jknehr closed 11 years ago

jknehr commented 11 years ago

Hi Peter,

Here's a quick version without holding the buffers in the list indefinitely. This will at least allow GC to clean these up.

Thanks for taking a look!

Regards, Jonathan

peter-lawrey commented 11 years ago

Part of the reason this hasn't been done is I have not found a way to ensure this does something useful i.e. we are not just cleaning up the ByteBuffers which are trivial in the overall scheme of things. If indeed it allows you to unmap the files, that is useful in reducing virtual memory size. If it only allows you to recycle ByteBuffers this will save around 100 bytes per buffer, but if those buffers are ever needed again, will cost you more virtual memory.

On 9 February 2013 01:34, jknehr notifications@github.com wrote:

Hi Peter,

Here's a quick version without holding the buffers in the list indefinitely. This will at least allow GC to clean these up.

Thanks for taking a look!

Regards,

Jonathan

You can merge this Pull Request by running

git pull https://github.com/jknehr/Java-Chronicle master

Or view, comment on, or merge it at:

https://github.com/peter-lawrey/Java-Chronicle/pull/18 Commit Summary

  • removed buffer lists

File Changes

  • M src/main/java/com/higherfrequencytrading/chronicle/impl/IndexedChronicle.javahttps://github.com/peter-lawrey/Java-Chronicle/pull/18/files#diff-0(51)

Patch Links:

peter-lawrey commented 11 years ago

I have taken your idea and made it configurable. Retaining the buffers is desirable as it improves performance if the Chronicle is re-read and the cost in terms of the heap is very small. The cost to virtual memory is high which is not an issue on 64-bit platforms, but on 32-bit platforms is not workable.

To address this issue for 32-bit platforms, I have made the default mapping size smaller (4 MB vs 128 MB) and it retains only one index and data buffer at a time. I have provided the option to turn this on or off if the default is not appropriate.

This fix will be in the 1.6 release and is available in 1.6-SNAPSHOT now.

mingfang commented 11 years ago

Peter, this is great. Also keep in mind that having only one buffer means the buffer can not be use for both reading and writing, specifically to different parts of the file. In our systems where have a Journal interface that wraps the Chronicle. Our Journal has a ReadableJournal and WritableJournal subclass to prevent using the same Chronicle from being read and written at the same time.

jknehr commented 11 years ago

Yes, thanks a lot, Peter!

As for using the same object for reads & writes, I don't think that the Chronicle was ever meant to be thread safe, but otherwise it should work in a single-threaded model.

peter-lawrey commented 11 years ago

It was specifically designed to be single threaded, partly for performance but mostly because I found a very low level issue where sharing memory mapped files between threads can result in inconsistencies in the mapping tables. i.e. occasionally random BUG errors. Java provides memory model guarantees for regular memory but not memory mappings.

The solution I have used which doesn't appear too inefficient (if a little inelegant) is to have a chronicle class for each thread. This is fine for a small number of controlled threads.