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
kernel: Remove batchpriority from kernel library #30083
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
utACK 04ffe40 🎪 Show signatureSignature:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 04ffe40, this is an obvious improvement
} | ||
} // End scope of ImportingNow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "kernel: Remove batchpriority from kernel library" (04ffe40)
I think it might be useful to keep the // End scope of ImportNow
comment here. If someone is adding new code at this point, it's possible they might want to add it with importing set to false, or importing set to true, so the comment would be a reminder that the value will change at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to preserve the comment or the scoping? I was on the fence about this, but decided to drop the scoping after the introduced RAII wrappers for the ECC context, which don't scope either: https://github.com/bitcoin/bitcoin/pull/29252/files#diff-dbfadb3e0332664bff298a329b1d27065d2decbbe43fd391388af18f5861c114R17
Besides, I don't think it is likely that his function will grow. It is finally scoped in a reasonably limited way now. If a more functionality is added, it should probably go in another function and removing the scoping as done here encourages that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #30083 (comment)
Whatever you prefer is good. I like getting rid of the nested scope, and the current PR is fine. I was just suggesting keeping the comment as a reminder that the import variable would be change to false at that point.
The current usage of ScheduleBatchPriority is not transparent. Once the thread scheduling is changed, it remains unchanged for the remainder of the thread's lifetime. So move the call from `ImportBlocks` to the init code where it is clearer that its effect lasts for the entire lifetime of the thread. Users of the kernel library might not expect `ImportBlocks` to have an influence on the thread it is called in. Particularly since it is only a compile time option and cannot be controlled at runtime. With this patch users of the kernel library can now choose their own scheduling policy.
04ffe40
to
d4b17c7
Compare
Thanks for the ACKs, should be trivial to re-ACK. Updated 04ffe40 -> d4b17c7 (rmKernelBatchPriority_0 -> rmKernelBatchPriority_1, compare)
|
ACK d4b17c7 📭 Show signatureSignature:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK d4b17c7, just added suggested comment since last review
Concept ACK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK d4b17c7, I have reviewed the code and it looks OK.
Post-merge utACK d4b17c7 |
Ported to the CMake-based build system in hebasto#202. |
The current usage of ScheduleBatchPriority is not transparent. Once the thread scheduling is changed, it remains unchanged for the remainder of the thread's lifetime. So move the call from
ImportBlocks
to the init code where it is clearer that its effect lasts for the entire lifetime of the thread.Users of the kernel library might not expect
ImportBlocks
to have an influence on the thread it is called in. Particularly since it is only a compile time option and cannot be controlled at runtime. With this patch users of the kernel library can now freely choose their own scheduling policy.This PR is easier reviewed with
git diff --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra
This PR is part of the libbitcoinkernel project.