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(core): add IntMapByDynamicHash V1 implement #2377

Merged
merged 7 commits into from
Dec 12, 2023

Conversation

conghuhu
Copy link
Contributor

@conghuhu conghuhu commented Dec 5, 2023

Purpose of the PR

Add IntMapByDynamicHash for high performance, lockless, concurrency secure hashmap.

Main Changes

Verifying these changes

Dynamic Capcitity

image

With Init Capcitity

image

  • 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

@conghuhu
Copy link
Contributor Author

conghuhu commented Dec 5, 2023

@imbajin PTAL

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 179 lines in your changes are missing coverage. Please review.

Comparison is base (bfe9fae) 66.31% compared to head (2820d8d) 64.77%.
Report is 1 commits behind head on master.

Files Patch % Lines
...hugegraph/util/collection/IntMapByDynamicHash.java 60.83% 127 Missing and 52 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2377      +/-   ##
============================================
- Coverage     66.31%   64.77%   -1.54%     
- Complexity      828      981     +153     
============================================
  Files           510      511       +1     
  Lines         42186    42643     +457     
  Branches       5840     5945     +105     
============================================
- Hits          27975    27624     -351     
- Misses        11445    12270     +825     
+ Partials       2766     2749      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@imbajin imbajin requested review from javeme and zyxxoo December 5, 2023 09:52
}
}

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.

expect to serialize it into a chunk memory

@imbajin
Copy link
Member

imbajin commented Dec 6, 2023

From line L735 ~ L972, some suggestions from Copilot:

  1. Handling InterruptedException: In the waitForAllResizers method of the ResizeContainer class, the InterruptedException is caught but completely ignored. This could potentially cause issues with thread interruption. In most cases, InterruptedException should either some operation in the catch block or propagate it further.
  2. Endless loop: Depending on the IntMapByDynamicHash.tableAt(currentArray, index) != RESIZED condition in the helpWithResizeWhileCurrentIndex method, Java could get stuck in an endless while loop. The condition that ends the while loop (change of IntMapByDynamicHash.tableAt(currentArray, index)) does not seem to be reachable and changeable within the loop.
  3. Concurrency: This code has complex concurrent operations and synchronizations, which are often a source of hard-to-detect bugs. Please ensure that your multithreading logic is correct and you are properly handling thread synchronization.
  4. Thread.yield() usage: The usage of Thread.yield() is generally not recommended since its behavior can be unpredictable and could vary with different JVMs. You would also want to avoid it for real-time systems.
  5. Null Entry objects: In the HashIterator's findNext method, null Entry objects are omitted. This would cause hasNext and next methods to not properly reflect the presence of these null entries.
  6. Mutation of shared resources: Since the array being operated on is not final nor encapsulated, external threads could affect the same array. This could cause unexpected behaviors if not properly handled.
  7. Potential multithreading issues: This code contains various multithreaded operations (e.g., asynchronous resizers updating in the ResizeContainer class), which can create complex interactions. Without proper synchronization controls, this may result in data inconsistencies.
  8. Method reverseTransfer is not in the code you provided: There's method reverseTransfer is called in helpWithResize method. However, the code for reverseTransfer is not provided and thus, can't be validated.

Please note that these are potential issues that could lead to bugs based on the provided code snippet. Whether they could actually pose problems depends on the exact workflow and environment in which this code is used. It would also benefit from unit-tests to assure that functionality is as expected.

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.

We merge this V1 version first for release 1.2.0, and link it to the summary issue (enhance it in the future)

@imbajin imbajin changed the title feat: add IntMapByDynamicHash feat(core): add IntMapByDynamicHash V1 implement Dec 11, 2023
@simon824 simon824 merged commit b52517c into apache:master Dec 12, 2023
19 of 21 checks passed
@imbajin imbajin added this to the 1.2.0 milestone Dec 12, 2023
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants