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

Optimized computation for block_len #50

Merged
merged 11 commits into from
Jun 14, 2022
Merged

Optimized computation for block_len #50

merged 11 commits into from
Jun 14, 2022

Conversation

kampersanda
Copy link
Member

This PR implemented computations for block_len in BuildHelper by using bit shift.

Copy link
Member

@vbkaisetsu vbkaisetsu left a comment

Choose a reason for hiding this comment

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

Hmm...
I think the benefit of this change is too small for increasing complexity of the struct.

First, multiplication is as efficient as shifting, or just a little slower.
Second, division is significantly slower, but it is a small difference compared to the overall processing.

@vbkaisetsu
Copy link
Member

@kampersanda btw, it would make more sense to store the number of blocks rather than the number of elements, wouldn't it?
The number of elements is always the multiply of the number of blocks, and multiplication is faster than division.

@kampersanda
Copy link
Member Author

@vbkaisetsu Makes sense. Your solution will allow for faster computation without increasing complexity.
Thanks for your suggestion. I'll implement it in this PR.

@kampersanda
Copy link
Member Author

I implemented 0544cbc.
@vbkaisetsu Could you review it again?

@vbkaisetsu
Copy link
Member

Need #52 before merging this

@vbkaisetsu vbkaisetsu merged commit 5fd97cf into main Jun 14, 2022
@vbkaisetsu vbkaisetsu deleted the opt-block-len branch June 14, 2022 02:42
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