use of java 1.4 assert

[Marcel Reutegger <ma...@gmx.net>]

hi all,

I'm currently working on an improvement regarding memory consumption in 
jackrabbit (replace reference map for listener in item states with a 
more light weight collection class). doing that, I would like to use the 
java 1.4 assert facility to include preconditions that make sure a 
listener is only registered once. that way I don't have to check every 
time if a listener is already registered.

using 1.4 asserts would required the '-source 1.4' switch when compiling 
jackrabbit. our maven project.properties already adds that switch. so, 
there would be nothing to do on our side. but probably a comment in 
documentation that talks about this.

per default asserts are disabled and will not impact performance.
when running the unit tests, asserts would then be enabled using the -ea 
switch for the test run.

what's the general feeling about this?

regards
  marcel

Re: use of java 1.4 assert

[Jukka Zitting <ju...@gmail.com>]

Hi,

On 2/16/06, Marcel Reutegger <ma...@gmx.net> wrote:
> what's the general feeling about this?

+1

BR,

Jukka Zitting

--
Yukatan - http://yukatan.fi/ - info@yukatan.fi
Software craftmanship, JCR consulting, and Java development

Re: use of java 1.4 assert

[Jukka Zitting <ju...@gmail.com>]

Hi,

I agree with Marcel's points. Using assert is not appropriate for
public methods like the JCR API and the o.a.j.api interface, as we
cannot control the code that accesses those methods. We can however
control the internal Jackrabbit code, and using asserts is a good way
to more strictly enforce the internal method interfaces.

Regardless of the performance points, my guideline for using assert has been:

 * if a method is only called from within the same source tree,
   use assert for preconditions
 * if a method is called by external components,
   explicitly check arguments and throw normal exceptions for preconditions

I'd also be happy to see assert getting gradually used also for things
like postconditions and loop invariants in the more complex methods.

BR,

Jukka Zitting

--
Yukatan - http://yukatan.fi/ - info@yukatan.fi
Software craftsmanship, JCR consulting, and Java development

Re: use of java 1.4 assert

[Marcel Reutegger <ma...@gmx.net>]

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
> 

Re: use of java 1.4 assert

[Tobias Bocanegra <to...@day.com>]

hi piyush,
this was a pure performane optimization. it actually _was_ a weak
hashset before. since the itemstate stuff is deeply in the core and
all internal, no everyday jackrabbit user/jsr170 developer should need
to hassle with an assertion exception.

regards, toby

On 2/22/06, Piyush Purang <pp...@gmail.com> wrote:
> Hi Marcel,
>
> I finally had time to look into your usage of assert and I can't convince
> myself that it is correct usage.
>
> The reason you decided to use an assert-statement is that on a production
> environment (PE) asserts will be turned off (which is a very likely
> scenario) and hence performance will be better as per the promise [1].
>
> 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.
>
> 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. 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.) 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.
>
> 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.
>
> 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.
>
> How about using a weak hash set?
>
> Cheers
> Piyush
>
> [1] http://java.sun.com/j2se/1.4.2/docs/guide/lang/assert.html
>
>  On 2/17/06, Marcel Reutegger <ma...@gmx.net> wrote:
> >
> > I've just committed a change that uses asserts in the sources.
> >
> > Users of IntelliJ have to manually change the language level of the
> > jackrabbit project to 1.4. It seems that the idea maven plugin does not
> > take the maven.compile.source=1.4 property into account when generating
> > a project file :-/
> >
> > regards
> >   marcel
> >
> > Marcel Reutegger wrote:
> > > hi all,
> > >
> > > I'm currently working on an improvement regarding memory consumption in
> > > jackrabbit (replace reference map for listener in item states with a
> > > more light weight collection class). doing that, I would like to use the
> > > java 1.4 assert facility to include preconditions that make sure a
> > > listener is only registered once. that way I don't have to check every
> > > time if a listener is already registered.
> > >
> > > using 1.4 asserts would required the '-source 1.4' switch when compiling
> > > jackrabbit. our maven project.properties already adds that switch. so,
> > > there would be nothing to do on our side. but probably a comment in
> > > documentation that talks about this.
> > >
> > > per default asserts are disabled and will not impact performance.
> > > when running the unit tests, asserts would then be enabled using the -ea
> > > switch for the test run.
> > >
> > > what's the general feeling about this?
> > >
> > > regards
> > >  marcel
> > >
> >
> >
>
>


--
-----------------------------------------< tobias.bocanegra@day.com >---
Tobias Bocanegra, Day Management AG, Barfuesserplatz 6, CH - 4001 Basel
T +41 61 226 98 98, F +41 61 226 98 97
-----------------------------------------------< http://www.day.com >---

Re: use of java 1.4 assert

[Piyush Purang <pp...@gmail.com>]

Hi Marcel,

I finally had time to look into your usage of assert and I can't convince
myself that it is correct usage.

The reason you decided to use an assert-statement is that on a production
environment (PE) asserts will be turned off (which is a very likely
scenario) and hence performance will be better as per the promise [1].

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.

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. 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.) 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.

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.

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.

How about using a weak hash set?

Cheers
Piyush

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

 On 2/17/06, Marcel Reutegger <ma...@gmx.net> wrote:
>
> I've just committed a change that uses asserts in the sources.
>
> Users of IntelliJ have to manually change the language level of the
> jackrabbit project to 1.4. It seems that the idea maven plugin does not
> take the maven.compile.source=1.4 property into account when generating
> a project file :-/
>
> regards
>   marcel
>
> Marcel Reutegger wrote:
> > hi all,
> >
> > I'm currently working on an improvement regarding memory consumption in
> > jackrabbit (replace reference map for listener in item states with a
> > more light weight collection class). doing that, I would like to use the
> > java 1.4 assert facility to include preconditions that make sure a
> > listener is only registered once. that way I don't have to check every
> > time if a listener is already registered.
> >
> > using 1.4 asserts would required the '-source 1.4' switch when compiling
> > jackrabbit. our maven project.properties already adds that switch. so,
> > there would be nothing to do on our side. but probably a comment in
> > documentation that talks about this.
> >
> > per default asserts are disabled and will not impact performance.
> > when running the unit tests, asserts would then be enabled using the -ea
> > switch for the test run.
> >
> > what's the general feeling about this?
> >
> > regards
> >  marcel
> >
>
>

Re: use of java 1.4 assert

[Marcel Reutegger <ma...@gmx.net>]

I've just committed a change that uses asserts in the sources.

Users of IntelliJ have to manually change the language level of the 
jackrabbit project to 1.4. It seems that the idea maven plugin does not 
take the maven.compile.source=1.4 property into account when generating 
a project file :-/

regards
  marcel

Marcel Reutegger wrote:
> hi all,
> 
> I'm currently working on an improvement regarding memory consumption in 
> jackrabbit (replace reference map for listener in item states with a 
> more light weight collection class). doing that, I would like to use the 
> java 1.4 assert facility to include preconditions that make sure a 
> listener is only registered once. that way I don't have to check every 
> time if a listener is already registered.
> 
> using 1.4 asserts would required the '-source 1.4' switch when compiling 
> jackrabbit. our maven project.properties already adds that switch. so, 
> there would be nothing to do on our side. but probably a comment in 
> documentation that talks about this.
> 
> per default asserts are disabled and will not impact performance.
> when running the unit tests, asserts would then be enabled using the -ea 
> switch for the test run.
> 
> what's the general feeling about this?
> 
> regards
>  marcel
> 

Re: use of java 1.4 assert

[Stefan Guggisberg <st...@gmail.com>]

On 2/16/06, Marcel Reutegger <ma...@gmx.net> wrote:
> what's the general feeling about this?

+1

cheers
stefan

>
> regards
>   marcel
>

Re: use of java 1.4 assert

[Tobias Bocanegra <to...@day.com>]

yeah +1

regards, tobi

On 2/16/06, Felix Meschberger <Fe...@day.com> wrote:
>
> >
> > what's the general feeling about this?
> +1
>
> Regards
> Felix
>
>


--
-----------------------------------------< tobias.bocanegra@day.com >---
Tobias Bocanegra, Day Management AG, Barfuesserplatz 6, CH - 4001 Basel
T +41 61 226 98 98, F +41 61 226 98 97
-----------------------------------------------< http://www.day.com >---

Re: use of java 1.4 assert

[Felix Meschberger <Fe...@day.com>]

>
> what's the general feeling about this?
+1

Regards
Felix