This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 168371 - ClassIndex$SPIListener allows duplicate listener registration, causes performance problem
Summary: ClassIndex$SPIListener allows duplicate listener registration, causes perform...
Status: NEW
Alias: None
Product: java
Classification: Unclassified
Component: Source (show other bugs)
Version: 6.x
Hardware: All All
: P4 blocker (vote)
Assignee: Svata Dedic
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-09 19:59 UTC by adrianriley
Modified: 2013-09-02 14:22 UTC (History)
0 users

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
ClassIndex using Set instead of List (30.47 KB, text/plain)
2009-07-16 19:10 UTC, adrianriley
Details

Note You need to log in before you can comment on or make changes to this bug.
Description adrianriley 2009-07-09 19:59:22 UTC
This is related to #167491. The class org.netbeans.api.java.source.ClassIndex$SPIListener does not use ChangeSupport 
to handle its listeners, but implements similar code itself (without using WeakReference, which might also be a 
problem). This allows duplicate registration of the same listener, resulting in multiple calls to the listener from 
typesChanged for a single event. Just as in ChangeSupport, this can cause a performance issue, particularly when the 
listener is added again during processing of an event. This seems a particular problem with 
org.netbeans.modules.j2ee.metadata.model.api.support.annotation.AnnotationModelHelper$ClassIndexListenerImpl. As I 
said in #167491, I don't see a reason for this behaviour.

Having modified the code myself to prevent duplicate registration here and in ChangeSupport, I finally have an 
implementation of Netbeans which is usable for days at a time, rather than getting slower and slower as it spends more 
and more time processing these event notifications.
Comment 1 Rastislav Komara 2009-07-13 10:23:19 UTC
Reassigning to parsing and indexing. It is really P2?
Comment 2 Vitezslav Stejskal 2009-07-14 12:28:46 UTC
I'm sorry for the confusion. This really belongs to java/source, but I'll have a closer look at what's going on there.
Comment 3 Vitezslav Stejskal 2009-07-14 13:17:38 UTC
Well according to http://java.sun.com/javase/technologies/desktop/javabeans/docs/spec.html "... the effects of adding
the same event listener object more than once on the same event source, or of removing an event listener object more
than once, or of removing an event listener object that is not registered, are all implementation defined ...". So
strictly speaking the implementation as it is now is ok. The listeners should make sure that they are attached only
once. On the other hand I don't see any problem with changing the implementation to only register each listener once.

Have you got a patch? Can you attach it here? Thanks
Comment 4 adrianriley 2009-07-16 19:10:47 UTC
Created attachment 84851 [details]
ClassIndex using Set instead of List
Comment 5 adrianriley 2009-07-16 19:17:20 UTC
Attached a file, modified to use a Set instead of a List. Given that it's much faster to traverse a CopyOnWriteArraySet
than to modify it, it might be worth checking listeners.contains(listener) in addClassIndexListener, i.e.:

    public void addClassIndexListener (final @NonNull ClassIndexListener listener) {
        assert listener != null;
        if (!listeners.contains(listener)) {
            listeners.add(listener);
        }
    }

I see that 167491 hasn't been looked at yet. That causes an even bigger performance issue.
Comment 6 adrianriley 2009-09-22 10:23:54 UTC
Having thought about this some more, I don't think allowing only one registration of a particular listener is the right
thing to do, since it does not cope with this type of thing:
     add ... add ... remove ... remove
So the options are to either ensure that all addListener calls have a matching removeListener, so copies of a listener
do not accumulate, and/or only notifying a listener of an event once, regardless of how many times it is registered.
Comment 7 David Strupl 2012-10-25 13:41:45 UTC
Bug prior to 7.0, not touched for the last 2 years --> P4.