jackrabbit-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marcel Reutegger <marcel.reuteg...@gmx.net>
Subject Re: use of java 1.4 assert
Date Thu, 23 Feb 2006 09:14:41 GMT
Piyush Purang wrote:
> The code was
> 
>  if (!listeners.containsKey(listener)) {
>             listeners.put(listener, listener);
> and now is
> 
> synchronized (listeners) {
>             assert (!listeners.contains(listener));
>             listeners.add (listener);
> 
> The previous version silently ignored requests to re-register listeners that
> are already registered. That is the client doesn't have to do any
> bookkeeping he can register umpteen times and he will be delivered the event
> just once.

most of the code already did register only once. there were two cases 
where multiple registration actually uncovered a coding error ;)

imo bookkeeping is not needed in this case. most listener instance are 
only registered to one entity. and adding / removing itself as a 
listener is then a matter of life cycle. when a ItemState is created it 
is added as listener on its overlaid ItemState and when it is destroyed 
it removes itself as listener.

> Now in the new version on the developer's box it will throw an
> AssertionError and the developer would have been strong-muscled into
> following the dictum register just once.

as a mentioned before this is a matter of life cycle and no special book 
keeping is needed.

> Most of the times though Listeners
> that would be registering themselves don't want to hassle theme selves in
> keeping tabs on how many times did they register. (At least I never
> programmed my listeners like that. I just registered.)

well, that's very convenient but adds overhead. in our case those 
listeners are at the very core of jackrabbit and get called a lot of 
times! Always having to check if a listener is already registered adds 
an overhead that is not justified for code that is not exposed at an api 
level.

> Hence there is a
> possibility that listeners on the PE get added multiple-times to the
> listener queue and receive the same event many times unless we again do some
> bookkeeping either on our side or listeners do it themselves (Performance
> will be hit). And no listener expects to be told about the same event more
> than once; that is something we all can agree on.

I agree, that's why I added the assert statement there ;)

> And if you still want to call it a pre-condition then [1] clearly points out
> don't use Assertions to implement pre-conditions for public methods.

that's because the documentation on that method in the document states 
that an IllegalArgumentException is thrown. therefore an assertion does 
not make sense there.

> Do you agree with my analysis? I of course don't have the kind of overview
> that you have of jackrabbit. Maybe my presumption that the outside world
> will be registering listeners is false. Even in that case I am not 100%
> convinced about using assertions to fit the role.

listeners on ItemState are not exposed to the outside world. ItemStates 
are internal data structures in jackrabbit.

> How about using a weak hash set?

that would more or less mean we go back to what we had before. HashSet's 
are usually implemented on top of a HashMap.
The problem with the ReferenceMap was that is consumes 4 times the 
memory than the WeakList we are using now. Additionally the current 
implementation is a bit faster even though it is implemented as list.

regards
  marcel

> Cheers
> Piyush
> 
> [1] http://java.sun.com/j2se/1.4.2/docs/guide/lang/assert.html
> 


Mime
View raw message