You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "JooHyukKim (via GitHub)" <gi...@apache.org> on 2023/07/24 00:27:44 UTC
[GitHub] [pulsar] JooHyukKim commented on pull request #20808: [improve][broker] Improve performance of `StatsOutputStream` by using `ArrayDeque` instead of `Stack`
JooHyukKim commented on PR #20808:
URL: https://github.com/apache/pulsar/pull/20808#issuecomment-1647022782
> Is this class on any critical path?
This won't make a crucial improvment. But since stats run at fixed rate, there will be a small, but certain improvement.
> Performance improvement often go with a convincible report instead of arbitrary best practice outside critical path - if these parts of code called only a few time during the whole application lifecycle, it's meaning less to improve the total time cost by a few CPU cycles.
>
> Also, do you verify whether this class will be called in concurrent context? Stack is thread safe while ArrayDeque is not. Without such analysis I'd reject such unproven "improvement" at risk of breaking existing semantic.
Right, I should have provided more context on this change. I added above in part `C. analysis` notes wrt to execution and concurrency. If explanation is not enough, feel free to close the PR! thank you 🙏🏼
/cc @tisonkun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org