Skip to content
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

Merged
merged 3 commits into from
Oct 26, 2023
Merged

feat(WIP): add IntMapByDynamicHash #2294

merged 3 commits into from
Oct 26, 2023

Conversation

conghuhu
Copy link
Contributor

@conghuhu conghuhu commented Aug 24, 2023

Purpose of the PR

  • close #xxx

Compared to jdk's concurrent HashMap, the effect has been improved:
image

Main Changes

New dynamic hash implementation for intMap, ensuring concurrency security.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #2294 (f7fd090) into master (4d7ad86) will decrease coverage by 1.08%.
Report is 6 commits behind head on master.
The diff coverage is 21.06%.

@@             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     
Files Changed Coverage Δ
...hugegraph/util/collection/IntMapByDynamicHash.java 0.00% <0.00%> (ø)
...ugegraph/util/collection/IntMapByDynamicHash2.java 42.34% <42.34%> (ø)

... 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
Copy link
Contributor

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.
*/
Copy link
Contributor

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 {
Copy link
Contributor

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;
Copy link
Contributor

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

@javeme
Copy link
Contributor

javeme commented Sep 10, 2023

Is it still in progress?

@conghuhu conghuhu changed the title feat: add IntMapByDynamicHash feat(WIP): add IntMapByDynamicHash Sep 10, 2023
@conghuhu
Copy link
Contributor Author

Is it still in progress?

Yes, I'm still working on it

}

@SuppressWarnings("UnusedDeclaration")
private volatile int size; // updated via atomic field updater
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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);
Copy link
Contributor

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;
Copy link
Contributor

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);
Copy link
Contributor

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) {
Copy link
Contributor

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) {
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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?

@SunnyBoy-WYH
Copy link
Contributor

SunnyBoy-WYH commented Oct 10, 2023

seems run rocksdb raft ci failed with timehout 6h, same with:

#2278

https://github.com/apache/incubator-hugegraph/actions/runs/6470766980/usage?pr=2278

@imbajin imbajin changed the base branch from master to tmp-map October 26, 2023 07:20
Copy link
Member

@imbajin imbajin left a 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

@imbajin imbajin merged commit e6f64db into apache:tmp-map Oct 26, 2023
17 of 21 checks passed
conghuhu added a commit to conghuhu/incubator-hugegraph that referenced this pull request Dec 5, 2023
simon824 pushed a commit that referenced this pull request Dec 12, 2023
* 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 <jin@apache.org>
VGalaxies pushed a commit to VGalaxies/incubator-hugegraph that referenced this pull request Jan 12, 2024
* 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 <jin@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement General improvement perf
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants