-
Notifications
You must be signed in to change notification settings - Fork 176
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
Improve MemoryPool locality #12
Conversation
Thanks Arthur. We have added a simple unit test for this at |
0060a81
to
6922f3f
Compare
That's very good, I wanted some tests while writing this. Although, I think they're not really good as 97+% of the cpu time is spend inside rand() and the fill pattern it's not deterministic. I updated the code a bit (second commit in this PR) trying to make it more repeatable and a bit less artificial. I had to increase the iterations a lot to mask the noise from spawning/joining the threads and the fill amount shouldn't be too long otherwise it's impossible to measure what we really want to measure.
This is probably the best case scenario for deque as most stdlib implementations will fit 8 items per node just fine and won't cause any malloc/free. Also, it hardly stress locality, as the benchmark quickly puts everything into L3 and L2. |
Thanks for the patch, Arthur! If this is your first code contribution to Couchbase, you'll need to fill out our Contributor License Agreement (http://review.couchbase.org/#/settings/agreements), which basically just declares that you created the code and are allowing us to release it under the Apache license. |
57d9b0b
to
af55b52
Compare
I just filled it and rebased the patch. |
Are you still interested in this? If yes I can rebase it and fix the conflicts. |
Yes, we are, sorry for the delay, Arthur. As it turns out I don't have the permission to merge pull requests. But I have notified the ones who do. Thanks again |
Improve cache locality by using a stack for indexes and a continuous block of memory.
I've rebased the commits on top of master. |
As reported by ASan: ==25822==ERROR: LeakSanitizer: detected memory leaks Direct leak of 721424 byte(s) in 11 object(s) allocated from: #0 0xffff9f6d18bc in malloc (/opt/gcc-10.2.0/lib64/libasan.so.6+0x9d8bc) #1 0x4b0284 in hash_init ../forestdb/src/hash.cc:32 #2 0x40e254 in _fname_create ../forestdb/src/blockcache.cc:806 #3 0x410698 in bcache_write ../forestdb/src/blockcache.cc:1063 #4 0x457dc8 in filemgr_read ../forestdb/src/filemgr.cc:2147 #5 0x4474d4 in _docio_read_through_buffer(docio_handle*, unsigned long, err_log_callback*, bool) ../forestdb/src/docio.cc:711 #6 0x4474d4 in _docio_read_length ../forestdb/src/docio.cc:803 #7 0x44c8c0 in docio_read_doc ../forestdb/src/docio.cc:1242 #8 0x4d6a3c in fdb_kvs_header_read ../forestdb/src/kv_instance.cc:1142 #9 0x485b94 in _fdb_open ../forestdb/src/forestdb.cc:2076 #10 0x4adfbc in _fdb_recover_compaction(_fdb_kvs_handle*, char const*) ../forestdb/src/forestdb.cc:585 #11 0x4869ac in _fdb_open ../forestdb/src/forestdb.cc:2237 #12 0x486f74 in fdb_open ../forestdb/src/forestdb.cc:848 #13 0x525da0 in compact_rename_to_original_test() ../forestdb/tests/functional/compact_functional_test.cc:525 #14 0x546878 in main ../forestdb/tests/functional/compact_functional_test.cc:3904 #15 0xffff9e4e2ce0 in __libc_start_main (/lib64/libc.so.6+0x1fce0) #16 0x404c4c (/home/couchbase/server/build/forestdb/tests/functional/compact_functional_test+0x404c4c) Direct leak of 721424 byte(s) in 11 object(s) allocated from: #0 0xffff9f6d18bc in malloc (/opt/gcc-10.2.0/lib64/libasan.so.6+0x9d8bc) #1 0x4b0284 in hash_init ../forestdb/src/hash.cc:32 #2 0x40e254 in _fname_create ../forestdb/src/blockcache.cc:806 #3 0x410698 in bcache_write ../forestdb/src/blockcache.cc:1063 #4 0x457dc8 in filemgr_read ../forestdb/src/filemgr.cc:2147 #5 0x4474d4 in _docio_read_through_buffer(docio_handle*, unsigned long, err_log_callback*, bool) ../forestdb/src/docio.cc:711 #6 0x4474d4 in _docio_read_length ../forestdb/src/docio.cc:803 #7 0x44c8c0 in docio_read_doc ../forestdb/src/docio.cc:1242 #8 0x4d6a3c in fdb_kvs_header_read ../forestdb/src/kv_instance.cc:1142 #9 0x485b94 in _fdb_open ../forestdb/src/forestdb.cc:2076 #10 0x486f74 in fdb_open ../forestdb/src/forestdb.cc:848 #11 0x524460 in compaction_delete_old_test() ../forestdb/tests/functional/compact_functional_test.cc:345 #12 0x546874 in main ../forestdb/tests/functional/compact_functional_test.cc:3903 #13 0xffff9e4e2ce0 in __libc_start_main (/lib64/libc.so.6+0x1fce0) #14 0x404c4c (/home/couchbase/server/build/forestdb/tests/functional/compact_functional_test+0x404c4c) Change-Id: I6179654af9da764d4ceed04fb2702ca84f43c874 Reviewed-on: http://review.couchbase.org/c/forestdb/+/159308 Tested-by: Build Bot <build@couchbase.com> Reviewed-by: Trond Norbye <trond.norbye@couchbase.com>
Improve cache locality by using a stack for indexes and a continuous block of memory.
Although it now requires a multiplication (at least preserving this API) the operations on the underlining vector are faster and have better locality than the previous underlining deque.