You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by mdykman <gi...@git.apache.org> on 2014/10/06 16:58:58 UTC
[GitHub] kafka pull request: changed List type in RequestPurgatory.Watchers
GitHub user mdykman opened a pull request:
https://github.com/apache/kafka/pull/34
changed List type in RequestPurgatory.Watchers
class Watchers is declared, having member
private val requests = new util.ArrayList[T]
in purgeSatisfied(), an iterator is created which conditionally removes elements from the list as it goes:
RequestPurgatory.scala, lines 193-201:
val iter = requests.iterator()
var purged = 0
while(iter.hasNext) {
val curr = iter.next
if(curr.satisfied.get()) {
iter.remove()
purged += 1
}
}
Using the .remove operation on an ArrayList iterator is very expensive as the ArrayList promises a contiguous backing array and all higher elements must be shifted on every operation.
A LinkedList is optimized for for the removal of arbitrary elements. Therefore recommending:
- private val requests = new util.ArrayList[T]
+ private val requests = new util.LinkedList[T]
This case was identified due to an observed failure In a high-load production environment, which found one or more cores pinned (ie. @100 usage) insider this loop. Once the pin was established, it tended to remain until the system was restarted.
As the loop is within a synchronized block, checkAndMaybeAdd (0.8.2) or add (0.8.1) which are also synchronized on the same object, may become blocked by this inefficiency.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mdykman/kafka 0.8.2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/kafka/pull/34.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #34
----
commit ade6abea822b87ac72309ecd6c43cfe4adc81e22
Author: Michael Dykman <md...@gmail.com>
Date: 2014-10-03T20:11:14Z
changed List type in RequestPurgatory.Worker
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] kafka pull request #34: changed List type in RequestPurgatory.Watchers
Posted by pono <gi...@git.apache.org>.
Github user pono closed the pull request at:
https://github.com/apache/kafka/pull/34
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] kafka pull request: changed List type in RequestPurgatory.Watchers
Posted by Jay Kreps <ja...@gmail.com>.
Hey Michael,
Nice catch. We had actually fixed this issue once already, but it looks
like the change was lost. Re-opened KAFKA-1430 to track:
https://issues.apache.org/jira/browse/KAFKA-1430
-Jay
On Mon, Oct 6, 2014 at 7:58 AM, mdykman <gi...@git.apache.org> wrote:
> GitHub user mdykman opened a pull request:
>
> https://github.com/apache/kafka/pull/34
>
> changed List type in RequestPurgatory.Watchers
>
> class Watchers is declared, having member
>
> private val requests = new util.ArrayList[T]
>
> in purgeSatisfied(), an iterator is created which conditionally
> removes elements from the list as it goes:
>
> RequestPurgatory.scala, lines 193-201:
>
> val iter = requests.iterator()
> var purged = 0
> while(iter.hasNext) {
> val curr = iter.next
> if(curr.satisfied.get()) {
> iter.remove()
> purged += 1
> }
> }
>
> Using the .remove operation on an ArrayList iterator is very expensive
> as the ArrayList promises a contiguous backing array and all higher
> elements must be shifted on every operation.
>
> A LinkedList is optimized for for the removal of arbitrary elements.
> Therefore recommending:
>
> - private val requests = new util.ArrayList[T]
> + private val requests = new util.LinkedList[T]
>
>
> This case was identified due to an observed failure In a high-load
> production environment, which found one or more cores pinned (ie. @100
> usage) insider this loop. Once the pin was established, it tended to
> remain until the system was restarted.
>
> As the loop is within a synchronized block, checkAndMaybeAdd (0.8.2)
> or add (0.8.1) which are also synchronized on the same object, may become
> blocked by this inefficiency.
>
> You can merge this pull request into a Git repository by running:
>
> $ git pull https://github.com/mdykman/kafka 0.8.2
>
> Alternatively you can review and apply these changes as the patch at:
>
> https://github.com/apache/kafka/pull/34.patch
>
> To close this pull request, make a commit to your master/trunk branch
> with (at least) the following in the commit message:
>
> This closes #34
>
> ----
> commit ade6abea822b87ac72309ecd6c43cfe4adc81e22
> Author: Michael Dykman <md...@gmail.com>
> Date: 2014-10-03T20:11:14Z
>
> changed List type in RequestPurgatory.Worker
>
> ----
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>