-
Notifications
You must be signed in to change notification settings - Fork 160
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
GH-2998 native store memory overflow performance #4974
base: develop
Are you sure you want to change the base?
GH-2998 native store memory overflow performance #4974
Conversation
|
|
After
|
@@ -68,52 +69,22 @@ class LmdbSailStore implements SailStore { | |||
private final ValueStore valueStore; | |||
|
|||
private final ExecutorService tripleStoreExecutor = Executors.newCachedThreadPool(); | |||
private final CircularBuffer<Operation> opQueue = new CircularBuffer<>(1024); | |||
private final ArrayBlockingQueue<Operation> opQueue = new ArrayBlockingQueue<>(1024); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenwenzel I found out that the ArrayBlockingQueue supports all the features that you need here and is more performant with a lot less busy waiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be wonderful. Do you have a benchmark for comparing this change?
I've switched to the custom buffer as it was much faster than blocking queues - at least in our write benchmarks.
LMDBBefore
After
|
That means we should just stay with the CircularBuffer? |
Ahh. That's ops per second and not ms/op which I use in my benchmarks. |
@kenwenzel IntelliJ is saying that there is a non atomic update to an atomic field. In general I'm a bit sceptical if this is even thread safe. If two threads both call add and proceed to |
@kenwenzel IntelliJ is saying that there is a non atomic update to an atomic field. In general I'm a bit sceptical if this is even thread safe. If two threads both call add and proceed to `elements[tail] = element` then one thread will overwrite the other. Is it supposed to be thread safe? In this specific case |
72de6f1
to
6668015
Compare
@hmottestad I would propose to keep the CircularBuffer for the LmdbStore. |
Yeah. I thought the same. I reverted it. I also reverted my attempt at throttling new transactions while a lot of write transactions are running. I'll try running the benchmarks again and see if things are better. The biggest improvement is still included, supporting addAll through the entire stack from the Changeset through to the store. |
4b15ecd
to
e60d672
Compare
LMDBBefore
After
@kenwenzel That's much better isn't it? Can you review my proposed changes? |
Looks impressive - especially for larger transactions..Cool :-) |
@kenwenzel I managed to get this error when running one of my benchmarks. |
I'll check this. Maybe gcIds is called from the wrong thread. |
OK, I think I found it. The method resizeMap expects always a surrounding readTransaction which is not the case for gcIds. |
I've tried about every permutation of the following
I've tried moving the write lock into the first "if" clause. I've tried == instead of !=. And all combinations of those. Either I end up with a deadlock or I end up with an exception. |
I suppose you also did the same for re-enabling the read-lock some lines below? |
I've pushed a commit that hopefully fixes the problem. |
689c0bb
to
7694fc9
Compare
I'm also getting GC overhead exception even though there is plenty of free memory:
|
I think this is the same error as the weird problem with the NativeStore. IO operations fail for some reason but are somehow swallowed in NativeStore. |
It seems to happen inside the new ID GC code where you fixed the lock issue by adding a read transaction. |
Yes, that's correct. The question is: Does it always happen if resizeMap is required within gcIds or only from time to time? |
@hmottestad I can't reproduce it with my branch Maybe there is some LMDB modification operation that results in an error like |
I reverted the changes you did to fix the read lock issue and added a try catch rollback to my benchmark code instead. I'm still running into this MDB_BAD_TXN issue. There also doesn't seem to be any way to handle the MDB_BAD_TXN issue, since even calling a rollback afterwards causes the entire JVM to exit with 134. |
@kenwenzel I have come to the conclusion that the ParallelGC is very hard to deal with when trying to predict that we are running low on memory. When I switched to G1GC things became a lot more consistent and I'm able to consistently overflow to disk before running out of memory. I also tried to increase the number of transactions in my benchmark and the performance scaled quite well and without running out of memory. |
The wrapping I've seen some places where Furthermore, we could also move |
I wrapped everything i could in E(...) and now the MDB_BAD_TXN doesn't happen anymore :) I probably wrapped more than is needed, but at least we now know that that is what the problem is. |
Thank you! A small note from my side: I would propose to not wrap the _get.methods as it will have a performance impact and the return values are already tested. But checking modifications (put, delete) is really good! |
4dfd13b
to
c9734dc
Compare
@@ -953,8 +958,10 @@ protected void deleteValueToIdMappings(MemoryStack stack, long txn, Collection<L | |||
for (Long id : ids) { | |||
idVal.mv_data(id2data(idBb.clear(), id).flip()); | |||
// id must not have a reference count and must have an existing value | |||
if (mdb_get(writeTxn, refCountsDbi, idVal, ignoreVal) != 0 && | |||
mdb_get(txn, dbi, idVal, dataVal) == 0) { | |||
int a = E(mdb_get(writeTxn, refCountsDbi, idVal, ignoreVal)); // this is where I get MDB_BAD_TXN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenwenzel I fairly consistently get MDB_BAD_TXN here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be due to a previous map full. Can you try to move resizeMap
into the inner loop of gcIds
?
@kenwenzel It does look like it's all due to the map becoming full. I couldn't find out where the map was getting full. I tried to add calls to resize a bunk of different places without any of it actually making any difference. In the end I changed up the resize code a bit where is checks if a resize is needed. Previously it tried to estimate how much space was used by the pages and how if the map would fit the new data or not. I've left that logic in as a fallback, but the main logic is now to just resize the map once it hits 80% full. Now I'm not seeing any more exceptions. |
If the map gets full then any write operation should usually return MDB_MAP_FULL. Due to the fact that LMDB uses MVCC you can't always know how much memory will be required in advance as it also depends on the number and access patterns of reader threads. Another challenge is also that you can't predict the number of page splits caused by a modification. |
I wasn't seeing any MDB_MAP_FULL so not sure what's actually going on. I was thinking of making the % limit configurable through a system property. Would probably be a good idea to make some of the constants in the MemoryOverflowModel configurable too. |
We can also just grow the db with a fill level of 50% or the like. It is only a "problem" under Windows were the mapped sizes are directly reflected in the file size. |
@@ -655,7 +669,7 @@ private long findId(byte[] data, boolean create) throws IOException { | |||
return null; | |||
} | |||
// id was not found, create a new one | |||
resizeMap(txn, 2 * data.length + 2 * (2 + Long.BYTES)); | |||
resizeMap(txn, 2L * data.length + 4L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 * (2 + Long.BYTES) is not equal to 4 -> it is equal to 2 * (2 + 8) = 20
@@ -698,7 +712,7 @@ private long findId(byte[] data, boolean create) throws IOException { | |||
return null; | |||
} | |||
|
|||
resizeMap(txn, 2 * data.length + 2 * (2 + Long.BYTES)); | |||
resizeMap(txn, 2L * data.length + 2L * 2L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here as above
@@ -698,7 +712,7 @@ private long findId(byte[] data, boolean create) throws IOException { | |||
return null; | |||
} | |||
|
|||
resizeMap(txn, 2 * data.length + 2 * (2 + Long.BYTES)); | |||
resizeMap(txn, 2L * data.length + 2L * 2L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
@@ -884,6 +886,19 @@ public boolean storeTriple(long subj, long pred, long obj, long context, boolean | |||
|
|||
if (stAdded) { | |||
for (int i = 1; i < indexes.size(); i++) { | |||
if (recordCache == null) { | |||
if (requiresResize()) { | |||
// map is full, resize required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this ever happen here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. It did. I thought it wouldn’t, since there is a check above, but it does trigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe requiresResize() should just consider more space that might be needed? Using the fill level could solve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that in this case here we might actually only require a resize because of one single operation that puts us over the 80% fill level. Could be an idea to have the requiresResize method take a priority argument, so that in this case here we can call it with low priority and then it will accept up to a fill level of 85% while if you call it with high priority it will have a fill level of 80%. Allows for some wiggle room. Or we can remove this check here and assume that a fill level of 80% should basically never cause any issues here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any figures about the performance impact of calling requiresResize
? I would go for a solution where we minimize these calls to reduce the impact especially within the hot paths like triple insertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't run any benchmarks other than the concurrent overflow benchmark which doesn't seem very affected by this.
GitHub issue resolved: #2998
Briefly describe the changes proposed in this PR:
PR Author Checklist (see the contributor guidelines for more details):
mvn process-resources
to format from the command line)