Skip to content

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