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(sqlite): SQLite Module for Hermes #248

Merged
merged 26 commits into from
Jun 18, 2024
Merged

feat(sqlite): SQLite Module for Hermes #248

merged 26 commits into from
Jun 18, 2024

Conversation

apskhem
Copy link
Collaborator

@apskhem apskhem commented Jun 6, 2024

Description

Add integration test for Hermes SQLite module for running on WASM runner.

Related Issue(s)

Closes #246

Description of Changes

  • Added integration test for the SQLite module in both functional tests and benchmarks
  • Added internal state for SQLite module. Without this, will cause error during WASM runtime

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@apskhem apskhem added the enhancement New feature or request label Jun 6, 2024
@apskhem apskhem added this to the M3: Hermes Essential Modules milestone Jun 6, 2024
@apskhem apskhem self-assigned this Jun 6, 2024
@apskhem apskhem linked an issue Jun 6, 2024 that may be closed by this pull request
@apskhem apskhem marked this pull request as draft June 6, 2024 14:11
@apskhem apskhem changed the title test(sqlite): WASM integration test module [WIP] test(sqlite): WASM integration test module Jun 6, 2024
@stevenj stevenj closed this Jun 6, 2024
@apskhem apskhem reopened this Jun 12, 2024
@apskhem apskhem changed the title [WIP] test(sqlite): WASM integration test module test(sqlite): WASM integration test module Jun 12, 2024
@apskhem apskhem marked this pull request as ready for review June 12, 2024 12:24
@apskhem apskhem added the review me PR is ready for review label Jun 12, 2024
@stevenj stevenj marked this pull request as draft June 13, 2024 08:06
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

Can you also add preliminary results from your benchmarks, comparing the speed of the in-memory sqlite db and the on-disk one?

Add a readme to the integration-test directory with a summary of the results of the benchmark. Make sure to include details of the machine it was run on. This way we have something to compare it to if we run it somewhere else.

wasm/integration-test/sqlite/src/lib.rs Outdated Show resolved Hide resolved
@stevenj stevenj changed the title test(sqlite): WASM integration test module feat(sqlite): SQLite Module for Hermes Jun 13, 2024
@apskhem apskhem requested a review from stevenj June 14, 2024 08:08
@apskhem apskhem marked this pull request as ready for review June 14, 2024 08:12
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy left a comment

Choose a reason for hiding this comment

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

Seems it's time add a generalised object which will maintain wasmtime::Resources.
So this abstraction will be used in both crypto and sqllite runtime extensions.
It could be something like:

trait ResourceAllocation {
   fn allocate<T>(params: T) -> Self {
    }
}

trait ResourceDeallocation {
   fn deallocate<T>(params: T) -> Self {
    }
}

struct ResourceManager<T> {
  apps_state: DashMap<HermesAppName, DashMap<u32, wasmtime::Resources<T>>>,
}

@apskhem
Copy link
Collaborator Author

apskhem commented Jun 14, 2024

Seems it's time add a generalised object which will maintain wasmtime::Resources. So this abstraction will be used in both crypto and sqllite runtime extensions. It could be something like:

trait ResourceAllocation {
   fn allocate<T>(params: T) -> Self {
    }
}

trait ResourceDeallocation {
   fn deallocate<T>(params: T) -> Self {
    }
}

struct ResourceManager<T> {
  apps_state: DashMap<HermesAppName, DashMap<u32, wasmtime::Resources<T>>>,
}

Generalizing this is a great idea, but it might be a bit out of scope at the moment. Once the all the modules are stable, we can revisit the idea of creating traits and structs for internal generalization with a dedicated ticket. As we have state using in cron, crypto, and sqlite modules, they are really have their own way of managing and storing states.

stevenj
stevenj previously approved these changes Jun 18, 2024
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenj stevenj merged commit 865bebf into main Jun 18, 2024
34 checks passed
@stevenj stevenj deleted the test/sqlite-module branch June 18, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request review me PR is ready for review
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

🛠️ [TASK] : Automated Test Cases running in CI.
3 participants