activemq-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tim Bain <tb...@alumni.duke.edu>
Subject Re: Setting expireMessagesPeriod=0 for advisory topics improved GC performance by 2x..
Date Sun, 10 May 2015 15:07:00 GMT
On Sat, May 9, 2015 at 9:41 AM, Kevin Burton <burton@spinn3r.com> wrote:

> > I wasn't at all clear why changing the frequency (to "never") of message
> > expiration checks would affect the performance of destination GC
> operations
> > when the number of queues is large.  Is that due to synchronization
> between
> > those two operations, or thread contention because one or both takes
> longer
> > than its periodicity to complete so we eventually have more pending
> threads
> > than the cores can run, or...
>
>
> You could look at the imp of purge() on a Timer.  It cause the entire queue
> to be rebuild and heapified again which is slow.
>
> Far far slower than synchronized.  The synchronized blocks aren’t even
> really showing up in my timings so I’m not really worried about them.  In
> fact removing them could introduce other issues so I”m hesitant to remove
> them without actual bugs.
>

What I wasn't understanding was that if we've enabled message expiration
checks, then when we GC a destination we have to cancel the message
expiration check timer (which we've chosen to do inefficiently by calling
purge()).  I finally see the connection between those two things.


> > I'm in favor of the changes as you've
> > described it (I can't think of a reason we'd want advisory messages to
> > expire, though ideally it would be configurable for just in case someone
> > has a need for it that I can't think of; configurable or not, having 0 as
> > the default sounds like a clear win to me), I'm just not clear on how it
> > has the larger effects you describe.
> >
> >
> We can go either route now.  My patches just refactored the way we remove
> task.  Now what I do is I just use an AtomicReference and I can clear() it
> when I want to remove the timer. The underlying timer will get executed,
> but since it checks first that it’s non-null it,s just a quick noop once
> it’s cleared.
>
>
> > Also, can you point me to the O(N^2) code related to Timer.purge()?  If
> it
> > really is purging every queue each time any queue's timer fires, that
> > sounds like incorrect code, but I'd like to see it to make sure I'm not
> > misunderstanding what you're describing.
> >
>
> I’m pretty sure it’s O(N^2)… what’s happening is that for each queue that
> needs to be GCd, it’s causing a purge.  So if you have 1000 queues, it has
> to purge and rebuild it… then if you have 999 queues, and you remove one,
> it has to do a full purge and rebuild each time.
>

I hadn't expected purge() to call heapify(), so I was just expecting O(N)
runtime for the actual removal.  The comments claim the runtime of the
purge operation is O(N + ClogN) where C is the number of deleted elements
(which would make it O(N) when C is 1, but looking at the code I think it's
actually O(N + ((N-C)/2)log((N-C)/2)), which is O(NlogN) when C is 1.  So
it's not O(N^2), but it's also definitely not O(N) despite the comment's
claim.

Now I just have it essentially flag the timer task as clear and it will go
> through the timer queue and not get executed.
>

Sounds fine, as long as "flag the timer task as clear" also means "call
TimerTask.cancel(), so that the task's state will be TimerTask.CANCELLED so
it'll be removed the next time it comes to the top of the queue in
Timer.mainLoop(), in addition to any WeakReference stuff we may do".  If
not, you're going to have a lot of no-op TimerTasks that stick around
forever, so 1) your value of N will grow over time, so even O(N) operations
will take longer over time, 2) there's still an expensive thread context
switch even if the task is a nominal no-op, and 3) eventually you could OOM
due to the memory those no-op TimerTasks take.  Does that match what you've
implemented?


> If you’re still interested I have screenshots from my profiling that I can
> pull up.
>
> I might submit these with my PR to back up my claims.
>
> Kevin
>
> --
>
> Founder/CEO Spinn3r.com
> Location: *San Francisco, CA*
> blog: http://burtonator.wordpress.com
> … or check out my Google+ profile
> <https://plus.google.com/102718274791889610666/posts>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message