You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/12/08 17:07:45 UTC
[GitHub] [kafka] divijvaidya opened a new pull request, #12967: MINOR: Replace ArrayBuffer with ListBuffer for better performance
divijvaidya opened a new pull request, #12967:
URL: https://github.com/apache/kafka/pull/12967
`ListBuffer` should be preferred over `ArrayBuffer` in cases where:
1. No indexing is required.
2. The data structure is converted to Java's List from scala.
`ListBuffer` has a [guaranteed constant time performance](https://docs.scala-lang.org/overviews/collections-2.13/performance-characteristics.html) for append operations which makes it a lucrative data structure for cases when we create a list and append multiple entries to it.
This PR modifies usage of `ArrayBuffer` to `ListBuffer` in latency sensitive code path. To prove the usefulness of this change, I am attaching a CPU profile for an [open messaging benchmark](https://openmessaging.cloud/docs/benchmarks/). Please note that the self-time of the `ReplicaManager.fetchMessages()` reduced after this changes (depicted by the empty space in top of the frame for `ReplicaManager.fetchMessages()`.
**Before this PR**
**After this PR**
Note: This PR piggybacks some general scala cleanup regarding usage of Optionals.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Replace ArrayBuffer with ListBuffer for better performance [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12967:
URL: https://github.com/apache/kafka/pull/12967#issuecomment-2563280007
This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] github-actions[bot] commented on pull request #12967: MINOR: Replace ArrayBuffer with ListBuffer for better performance
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12967:
URL: https://github.com/apache/kafka/pull/12967#issuecomment-1603639665
This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch) <p> If this PR is no longer valid or desired, please feel free to close it. If no activity occurrs in the next 30 days, it will be automatically closed.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Replace ArrayBuffer with ListBuffer for better performance [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12967: MINOR: Replace ArrayBuffer with ListBuffer for better performance
URL: https://github.com/apache/kafka/pull/12967
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #12967: MINOR: Replace ArrayBuffer with ListBuffer for better performance
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12967:
URL: https://github.com/apache/kafka/pull/12967#issuecomment-1343464276
> The data structure is converted to Java's List from scala.
Why is this? Java's `ArrayList` is array backed, just like `ArrayBuffer` while `ListBuffer` is a linked list.
Generally, linked lists are worse due to cache effects and `ArrayBuffer` has amoritzed constant append too.
One case where `ListBuffer` is better is when `ArrayBuffer` usage results in unnecessary allocations (i.e. small lists). A simple way of solving that for `ArrayBuffer` is to size the underlying array to be smaller.
I haven't looked at the results in detail yet, but I wanted to share this as I am a bit confused about the PR description.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12967: MINOR: Replace ArrayBuffer with ListBuffer for better performance
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12967:
URL: https://github.com/apache/kafka/pull/12967#discussion_r1043889697
##########
core/src/main/scala/kafka/coordinator/group/GroupMetadata.scala:
##########
@@ -484,10 +484,7 @@ private[group] class GroupMetadata(val groupId: String, initialState: GroupState
* group does not know, because the information is not available yet or because the it has
* failed to parse the Consumer Protocol, it returns true to be safe.
*/
- def isSubscribedToTopic(topic: String): Boolean = subscribedTopics match {
- case Some(topics) => topics.contains(topic)
- case None => true
- }
+ def isSubscribedToTopic(topic: String): Boolean = subscribedTopics.forall(topics => topics.contains(topic))
Review Comment:
How is this related to the PR description? Seems like an unrelated clean-up? Is your before/after test including these changes? Many of them _can_ result in additional allocations since you're replacing pattern matching with lambas.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org