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 feature 'multithread' to use Rsonpath in multithread context #622

Merged
merged 3 commits into from
Dec 24, 2024

Conversation

azoyan
Copy link
Contributor

@azoyan azoyan commented Dec 24, 2024

Short description

Improve the change from #618 to instead use Arc references and make MainEngine both Sync and Send again.

Issue

Resolves: #620

Checklist

All of these should be ticked off before you submit the PR.

  • I ran just verify locally and it succeeded.
  • Issue was given go ahead and is linked above OR I have included justification for a minor change.
  • Unit tests for my changes are included OR no functionality was changed.

Copy link
Member

@V0ldek V0ldek left a comment

Choose a reason for hiding this comment

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

Hey, thanks for taking the time to implement this on your own.

I think a separate feature is overkill, I'd rather just change the strings to Arc. The overhead for cloning those is negligble as it only happens during query compilation, accessing through Rc and Arc is the same, so it's fine.

Additionally, I'd feel better if we had a way of guarding against accidentally removing Send and Sync from this. We already have static_assertions as a dependency, so it'd be great if you could add a line that asserts MainEngine implements Send and Sync.

@azoyan
Copy link
Contributor Author

azoyan commented Dec 24, 2024

Hey, thanks for taking the time to implement this on your own.

I think a separate feature is overkill, I'd rather just change the strings to Arc. The overhead for cloning those is negligble as it only happens during query compilation, accessing through Rc and Arc is the same, so it's fine.

Additionally, I'd feel better if we had a way of guarding against accidentally removing Send and Sync from this. We already have static_assertions as a dependency, so it'd be great if you could add a line that asserts MainEngine implements Send and Sync.

I did it and need your code review.

Copy link
Member

@V0ldek V0ldek left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@V0ldek V0ldek enabled auto-merge (squash) December 24, 2024 01:46
@V0ldek V0ldek disabled auto-merge December 24, 2024 01:46
@V0ldek V0ldek enabled auto-merge (squash) December 24, 2024 01:48
@V0ldek V0ldek merged commit 37e9076 into rsonquery:main Dec 24, 2024
49 checks passed
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.

Add Rc to Arc feature in async and multithread context
2 participants