4:57 p.m.
Hi,
There is place for some performance optimization with respect to the
notification mechanism, in the Notifier class:
1) there are defensive Maps being created for being passed in the
pre/post notifications, even if no listeners are registered.
2) even if we do not have listeners for a certain event, there is still
a lookup in the listeners map to check for the available list of listeners
Optimizations:
for 1) - do not trigger a notification if no listener registered, use a
guard - similar to log.isTraceEnabled()
for 2) - rather than keeping a map with listeners registered for each
annotation type, rather keep the listeners list as a member - this way
we'll skip the list lookup.
Another thing that just come into my mind is not iterating on the list
of listeners but rather use sequential access - small gain though.
I expect this would gain us some performance, especially in the
standalone mode.
WDYT?
Cheers,
Mircea
5:01 a.m.
My comments below:
On 20 May 2008, at 22:57, Mircea Markus wrote:
Hi,
There is place for some performance optimization with respect to the
notification mechanism, in the Notifier class:
1) there are defensive Maps being created for being passed in the
pre/post notifications, even if no listeners are registered.
Are you looking at trunk? I can see that the defensive copy is only
made *after* checking for whether a listener exists.
E.g., in notifyNodeModified():
if (listeners != null && !listeners.isEmpty())
{
boolean originLocal = ctx.isOriginLocal();
Map dataCopy = copy(data, useMarshalledValueMaps);
... etc ...
}
2) even if we do not have listeners for a certain event, there is
still a lookup in the listeners map to check for the available list
of listeners
Yes, you mean, for example,
List<ListenerInvocation> listeners =
listenerInvocations.get(NodeModified.class);
in notifyNodeModified().
Since we have a finite set of callbacks, I don't mind these lists
becoming members as you suggested. The only thing is, they should be
final and never null, since they need to be threadsafe. Constructing
them lazily will become a problem without synchronizations.
Optimizations:
for 1) - do not trigger a notification if no listener registered,
use a guard - similar to log.isTraceEnabled()
for 2) - rather than keeping a map with listeners registered for
each annotation type, rather keep the listeners list as a member -
this way we'll skip the list lookup.
Another thing that just come into my mind is not iterating on the
list of listeners but rather use sequential access - small gain
though.
Well - looking at the implementation of a CopyOnWriteArrayList's
iterator, it is nothing but sequential access. The only gain would be
saving the construction of the iterator object and in wrapping array
accesses in a method (iterator.next()). For the sake of readability
though I'd rather stick with the foreach loop.
I expect this would gain us some performance, especially in the
standalone mode.
WDYT?
Cheers,
Mircea
_______________________________________________
jbosscache-dev mailing list
jbosscache-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/jbosscache-dev
--
Manik Surtani
Lead, JBoss Cache
manik(a)jboss.org
6:38 a.m.
Manik Surtani wrote:
My comments below:
On 20 May 2008, at 22:57, Mircea Markus wrote:
> Hi,
>
> There is place for some performance optimization with respect to the
> notification mechanism, in the Notifier class:
> 1) there are defensive Maps being created for being passed in the
> pre/post notifications, even if no listeners are registered.
Are you looking at trunk? I can see that the defensive copy is only
made *after* checking for whether a listener exists.
E.g., in notifyNodeModified():
if (listeners != null && !listeners.isEmpty())
{
boolean originLocal = ctx.isOriginLocal();
Map dataCopy = copy(data, useMarshalledValueMaps);
... etc ...
}
I am referring to the maps which are being passed to the notifier, not
the one that are being build within the notifier itself.
E.g.PutDataMapCommand
Map existingData = nodeSPI.getDataDirect();
if (!existingData.isEmpty())
{
oldData = new HashMap(existingData); // defensive copy
}
> 2) even if we do not have listeners for a certain event, there is
> still a lookup in the listeners map to check for the available list
> of listeners
Yes, you mean, for example,
List<ListenerInvocation> listeners =
listenerInvocations.get(NodeModified.class);
in notifyNodeModified().
Since we have a finite set of callbacks, I don't mind these lists
becoming members as you suggested. The only thing is, they should be
final and never null, since they need to be threadsafe. Constructing
them lazily will become a problem without synchronizations.
ok.
> Optimizations:
> for 1) - do not trigger a notification if no listener registered, use
> a guard - similar to log.isTraceEnabled()
> for 2) - rather than keeping a map with listeners registered for each
> annotation type, rather keep the listeners list as a member - this
> way we'll skip the list lookup.
>
> Another thing that just come into my mind is not iterating on the
> list of listeners but rather use sequential access - small gain though.
Well - looking at the implementation of a CopyOnWriteArrayList's
iterator, it is nothing but sequential access. The only gain would be
saving the construction of the iterator object and in wrapping array
accesses in a method (iterator.next()). For the sake of readability
though I'd rather stick with the foreach loop.
Haven't noticed that this is an CopyOnWriteArrayList, I agree with you
about readability.
> I expect this would gain us some performance, especially in the
> standalone mode.
> WDYT?
>
> Cheers,
> Mircea
> _______________________________________________
> jbosscache-dev mailing list
> jbosscache-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/jbosscache-dev
--
Manik Surtani
Lead, JBoss Cache
manik(a)jboss.org
9:48 a.m.
On 21 May 2008, at 12:38, Mircea Markus wrote:
Manik Surtani wrote:
> My comments below:
>
> On 20 May 2008, at 22:57, Mircea Markus wrote:
>
>> Hi,
>>
>> There is place for some performance optimization with respect to
>> the notification mechanism, in the Notifier class:
>> 1) there are defensive Maps being created for being passed in the
>> pre/post notifications, even if no listeners are registered.
>
> Are you looking at trunk? I can see that the defensive copy is
> only made *after* checking for whether a listener exists.
>
> E.g., in notifyNodeModified():
>
> if (listeners != null && !listeners.isEmpty())
> {
> boolean originLocal = ctx.isOriginLocal();
> Map dataCopy = copy(data, useMarshalledValueMaps);
> ... etc ...
> }
>
I am referring to the maps which are being passed to the notifier,
not the one that are being build within the notifier itself.
E.g.PutDataMapCommand
Map existingData = nodeSPI.getDataDirect();
if (!existingData.isEmpty())
{
oldData = new HashMap(existingData); // defensive copy
}
Wow yes, that is completely pointless.
--
Manik Surtani
Lead, JBoss Cache
manik(a)jboss.org
6033
days inactive
6034
days old
jbosscache-dev@lists.jboss.org
3 comments
2 participants
participants (2)
-
Manik Surtani -
Mircea Markus