-
Notifications
You must be signed in to change notification settings - Fork 525
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
feat(WIP): add IntMapByDynamicHash #2294
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2294 +/- ##
============================================
- Coverage 65.07% 64.00% -1.08%
- Complexity 979 987 +8
============================================
Files 498 502 +4
Lines 41240 42304 +1064
Branches 5738 5982 +244
============================================
+ Hits 26837 27075 +238
- Misses 11743 12502 +759
- Partials 2660 2727 +67
... and 18 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
} | ||
|
||
@SuppressWarnings("UnusedDeclaration") | ||
private volatile int size; // updated via atomic field updater |
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.
improve the style
/* | ||
we want 7 extra slots and 64 bytes for each | ||
slot. int is 4 bytes, so 64 bytes is 16 ints. | ||
*/ |
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.
improve the style
} | ||
} | ||
|
||
private static class Entry { |
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.
we expect a primitive type instead of a class, it's too many objects because each Entry will be generated an object
AtomicReferenceFieldUpdater.newUpdater(IntMapByDynamicHash.class, Entry[].class, | ||
"table"); | ||
|
||
private volatile Entry[] table; |
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.
expect a primitive type array here
Is it still in progress? |
Yes, I'm still working on it |
} | ||
|
||
@SuppressWarnings("UnusedDeclaration") | ||
private volatile int size; // updated via atomic field updater |
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.
move to the front of the class
} | ||
|
||
private static int tableSizeFor(int c) { | ||
int n = c - 1; |
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.
rename c/n to readable words?
|
||
@Override | ||
public void clear() { | ||
long[] currentArray = this.table; |
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.
ditto
while (true) { | ||
int length = currentArray.length; | ||
int index = this.hash(extractKey(toCopyEntry), length); | ||
long o = IntMapByDynamicHash2.tableAt(currentArray, index); |
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.
rename to value?
} | ||
|
||
private void reverseTransfer(long[] src, ResizeContainer resizeContainer) { | ||
long[] dest = resizeContainer.nextArray; |
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.
keep destArray style?
private void resize(long[] oldTable, int newSize) { | ||
int oldCapacity = oldTable.length; | ||
int end = oldCapacity - 1; | ||
long last = IntMapByDynamicHash2.tableAt(oldTable, end); |
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.
also keep lastTable style?
* Enable the current thread to participate in the expansion | ||
*/ | ||
private long[] helpWithResize(long[] currentArray) { | ||
if (resizeContainer == null) { |
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.
this.resizeContainer
} | ||
} | ||
|
||
private long[] helpWithResizeWhileCurrentIndex(long[] currentArray, int index) { |
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.
FromCurrentIndex?
return this.slowRemove(key, currentTable) != NULL_VALUE; | ||
} | ||
|
||
long e = o; |
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.
also add a fastRemove method?
if (o == RESIZED || o == RESIZING) { | ||
return this.slowGet(key, currentTable); | ||
} | ||
long e = o; |
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.
also add a fastGet method?
seems run rocksdb raft ci failed with timehout 6h, same with: https://github.com/apache/incubator-hugegraph/actions/runs/6470766980/usage?pr=2278 |
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.
Merge it now & address the comment & problems later
* feat(WIP): add IntMapByDynamicHash (#2294) * feat: add values & keys in IntMapByDynamicHash * add some basic comment & fix some style * feat: fix pr review * fix: fix some review --------- Co-authored-by: imbajin <[email protected]>
* feat(WIP): add IntMapByDynamicHash (apache#2294) * feat: add values & keys in IntMapByDynamicHash * add some basic comment & fix some style * feat: fix pr review * fix: fix some review --------- Co-authored-by: imbajin <[email protected]>
Purpose of the PR
Compared to jdk's concurrent HashMap, the effect has been improved:
Main Changes
New dynamic hash implementation for intMap, ensuring concurrency security.
Verifying these changes
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODO
Doc - Done
Doc - No Need