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

Add opt-in multithreading #85

Merged
merged 20 commits into from
Nov 26, 2023
Merged

Add opt-in multithreading #85

merged 20 commits into from
Nov 26, 2023

Conversation

matthewkeil
Copy link
Contributor

@matthewkeil matthewkeil commented Nov 5, 2023

We have a use case for utilizing our level db from multiple worker threads. The base library did most of the heavy lifting and this PR allows the bindings to be used across workers.

Methodology

Context-awareness was preserved and so was threadsafe operation from the node.js perspective. The open and close functionality was all that needed to change and a single mutex protects the setup. A flag was added so the feature is non-breaking and only provides additional functionality.

The open_db function searches for a record in a db_handles map, and if it exists the open_handle_count is incremented for that database. This was used to allow for multiple databases to be accessed per process. The reverse happens when the destructor or the closed_db method is called. If the handle is the last to be closed then the db instance is deleted from the map to cleanup the native LOCK.

@matthewkeil matthewkeil marked this pull request as ready for review November 6, 2023 15:36
@vweevers
Copy link
Member

vweevers commented Nov 6, 2023

Interesting! I didn't realize we were this close to supporting this use case.

What's your reasoning for making this opt-in (through allowMultiThreading)?

@matthewkeil
Copy link
Contributor Author

Interesting! I didn't realize we were this close to supporting this use case.

What's your reasoning for making this opt-in (through allowMultiThreading)?

Mostly so the change was non-breaking for existing users @vweevers. Before I implemented the flag, a few of the unit tests broke so assumed there might be users out there somewhere that are also counting on that behavior. The flag can be stripped out though or I suppose can default to true

@matthewkeil
Copy link
Contributor Author

There is a workflow issue due to python issue on macos.

https://stackoverflow.com/questions/77233855/why-did-i-got-an-error-modulenotfounderror-no-module-named-distutils

Would you like that fixed @vweevers?

vweevers added a commit that referenced this pull request Nov 6, 2023
@vweevers
Copy link
Member

vweevers commented Nov 6, 2023

Fixed in main: 3b1a6f2

@matthewkeil
Copy link
Contributor Author

matthewkeil commented Nov 6, 2023

Fixed in main: 3b1a6f2

Would you mind re-running the CI please @vweevers ?

@vweevers
Copy link
Member

vweevers commented Nov 6, 2023

You'll need to rebase your branch to get the fix.

@matthewkeil
Copy link
Contributor Author

You'll need to rebase your branch to get the fix.

Thanks for the quick fix @vweevers. Rebased!!

@vweevers
Copy link
Member

vweevers commented Nov 6, 2023

The flag can be stripped out though

Yeah. We can do that in a future semver-major version. It would simplify the code a bit, but I think it's good to introduce this feature gradually.

binding.cc Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
binding.cc Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@matthewkeil
Copy link
Contributor Author

@vweevers I think I got all of your comments addressed. Please let me know if there is anything else that you would like updated.

@vweevers
Copy link
Member

vweevers commented Nov 7, 2023

It might be surprising that with multithreading: false a handle is still created. E.g. in this example from the tests:

const location = tempy.directory()
const db1 = new ClassicLevel(location)
await db1.open()
t.is(db1.location, location)

One would expect to have exclusive access to this db location. But the code that follows shows the opposite:

const db2 = new ClassicLevel(location)
await db2.open({ multithreading: true })
t.is(db2.location, location)

In other words, I think multithreading: false should also mean "don't allow others to open this db location". Closer to current behavior.

@matthewkeil
Copy link
Contributor Author

matthewkeil commented Nov 8, 2023

In other words, I think multithreading: false should also mean "don't allow others to open this db location". Closer to current behavior.

Sounds good!! Updated to achieve that behavior @vweevers. I also added another test case to verify the fix

binding.cc Outdated Show resolved Hide resolved
Copy link
Member

@vweevers vweevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I'll wait with merging for a few days, to give others a chance to review (@juliangruber @ralphtheninja if you're able 🙏).

@matthewkeil
Copy link
Contributor Author

Nice work! I'll wait with merging for a few days, to give others a chance to review (@juliangruber @ralphtheninja if you're able 🙏).

Hi there @vweevers! Was curious if @juliangruber or @ralphtheninja were interested in taking a bite at the review apple? I will be happy to respond to any comments or concerns.

@ralphtheninja
Copy link
Member

Aaah! So basically we have a layer inbetween db instances and the corresponding location. Looks good!

@matthewkeil
Copy link
Contributor Author

Aaah! So basically we have a layer inbetween db instances and the corresponding location. Looks good!

Thanks @ralphtheninja! Much appreciated!! @juliangruber do you have any comments or concerns? @vweevers was curious before merging this... Just a nudge so we can get this imported into the chainsafe/lodestar repo

@vweevers
Copy link
Member

That'll do.

@vweevers vweevers changed the title Worker Multi-Threading Add opt-in multithreading Nov 26, 2023
@vweevers vweevers merged commit 7d497a5 into Level:main Nov 26, 2023
15 checks passed
@matthewkeil
Copy link
Contributor Author

matthewkeil commented Nov 26, 2023

@vweevers
Copy link
Member

vweevers commented Nov 26, 2023

Could be a race issue; can you check? I'll wait with the npm release.

For future reference (when the logs are deleted), the error is:

# check multithreading flag works as expected
not ok 5699 Error: Database is not open
  ---
    operator: error
    stack: |-
      Error: Database is not open
          at maybeOpened (/home/runner/work/classic-level/classic-level/node_modules/abstract-level/abstract-level.js:133:18)
          at /home/runner/work/classic-level/classic-level/node_modules/abstract-level/abstract-level.js:160:13
          at process.processTicksAndRejections (node:internal/process/task_queues:78:11)
  ...
not ok 5700 plan != count
  ---
    operator: fail
    expected: 9
    actual:   1
    stack: |-
      Error: plan != count
          at Test.assert [as _assert] (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:479:48)
          at Test.fail (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:573:7)
          at completeEnd (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:394:9)
          at next (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:404:4)
          at Test._end (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:424:2)
          at Test.end (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:198:7)
          at onError (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:133:10)
          at process.processTicksAndRejections (node:internal/process/task_queues:96:5)
  ...

@matthewkeil
Copy link
Contributor Author

Could be a race issue; can you check? I'll wait with the npm release

Yep. Will take a look this evening

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.

3 participants