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: Use ajv-threads package to parallelize jsonSchema validation of ModelInstanceDocuments #3176

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

stbrody
Copy link
Contributor

@stbrody stbrody commented Feb 29, 2024

No description provided.

@stbrody stbrody self-assigned this Feb 29, 2024
@stbrody stbrody force-pushed the threadedSchemaValidation branch 2 times, most recently from 4288fe2 to dfc524e Compare March 1, 2024 00:40
@stbrody stbrody requested review from dbcfd and dav1do March 1, 2024 02:09
@stbrody stbrody changed the title feat: Use worker threads for jsonSchema validation of ModelInstanceDocuments feat: Use ajv-threads package for jsonSchema validation of ModelInstanceDocuments Mar 1, 2024
Copy link
Contributor

@dav1do dav1do left a comment

Choose a reason for hiding this comment

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

LGTM. I ran a benchmark on my mac and saw create/sync in the new MIDs benchmark of 247.23/129.53 with this branch vs 220.48/144.25 with dev (only over a one minute run to make sure things didn't change substantially).

This will make schema validation in js-ceramic truly parallel for the first time
@stbrody stbrody changed the title feat: Use ajv-threads package for jsonSchema validation of ModelInstanceDocuments feat: Use ajv-threads package to parallelize jsonSchema validation of ModelInstanceDocuments Mar 12, 2024
@JulissaDantes
Copy link
Contributor

Why havent this been merged?

@stbrody
Copy link
Contributor Author

stbrody commented Mar 20, 2024

Why havent this been merged?

We said that we want to see that this will improve performance before merging it. So far our testing has shown this PR having no impact on performance/throughput whatsoever, which is surprising, so we want to understand what's happening first.

Personally I'd be fine with merging and then iterating in develop, but I don't feel strongly and I believe @nathanielc prefers to wait.

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