Skip to content

remove redundant Bit::SetBit #14226

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 7, 2024
Merged

remove redundant Bit::SetBit #14226

merged 3 commits into from
Oct 7, 2024

Conversation

flashmouse
Copy link
Contributor

I think these lines could be removed because each Bit::SetBit already call Bit::Finalize

@Mytherin
Copy link
Collaborator

Mytherin commented Oct 7, 2024

Thanks for the PR!

Good observation. I agree that this looks correct - but rather than removing the call to Bit::Finalize, can we turn the Bit::SetBit calls into Bit::SetBitInternal instead? Finalizing for every bit set is unnecessarily wasteful when we can do it only once after the operation has completed.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft October 7, 2024 11:40
@flashmouse flashmouse marked this pull request as ready for review October 7, 2024 11:41
@flashmouse flashmouse marked this pull request as draft October 7, 2024 13:06
@flashmouse flashmouse marked this pull request as ready for review October 7, 2024 13:41
@flashmouse
Copy link
Contributor Author

Thanks for the PR!

Good observation. I agree that this looks correct - but rather than removing the call to Bit::Finalize, can we turn the Bit::SetBit calls into Bit::SetBitInternal instead? Finalizing for every bit set is unnecessarily wasteful when we can do it only once after the operation has completed.

It's indeed a better solution, seems it could improve execute speed a bit, I have commited new code by your advice, thanks!

not quite sure what happend with the failed regression tests.

@Mytherin Mytherin merged commit 5a9a382 into duckdb:main Oct 7, 2024
41 of 42 checks passed
@Mytherin
Copy link
Collaborator

Mytherin commented Oct 7, 2024

Those seem unrelated. Thanks!

@flashmouse flashmouse deleted the rm_redundant_setbit branch October 13, 2024 04:56
github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Oct 16, 2024
@krlmlr
chore: Update vendored sources to duckdb/duckdb@5a9a382
9c29eea 
remove redundant Bit::SetBit (duckdb/duckdb#14226)
@github-actions github-actions bot mentioned this pull request Oct 16, 2024
Merged
github-actions bot added a commit to duckdb/duckdb-r that referenced this pull request Oct 16, 2024
@github-actions @krlmlr
chore: Update vendored sources to duckdb/duckdb@5a9a382 (#488)
9039737 
remove redundant Bit::SetBit (duckdb/duckdb#14226)

Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Changes Requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@flashmouse @Mytherin