Created attachment 52038 [details]
threaddump 1

Created attachment 52039 [details]
threaddump 2 - totally frozen ide

I'm going to have a look at this one.

Could you attach JSP file you are testing this on. My one seems to be reasonably quick.

I've got the file form ehucka offline and I can reproduce the problem.

Two problems. First one is caused by fix of issue 50923 (btw. it was 40_HR_FIX) which delays JSP parser for *15* seconds
in case JSP file is not open in editor. I could not get hold of marek to get more details on this as he was the one to
fix 50923. In this case though JSP includes four JSP fragments and parsing of each of them is dealyed for 15 seconds
(because they are not open in editor).

Second problem is that after 4*15seconds delay the Java file is generated from JSP and lexer/editor gets into infinite
loop while parsing it and IDE has to be killed. I have not filed P1 yet and waiting on editor guys to analyze logs I
sent them. However, at the moment they are busy with some other P1s.

One more issue is that Java file generated from JSP is uncompilable. tslota explained that JSP fragments cannot #1)
reference variables defined in main JSP and #2) cannot have any imports. That means that JSP file ehucka is testing on
is invalid. (?) I need to check JSP spec on this.

| One more issue is that Java file generated from JSP is uncompilable. tslota explained that JSP fragments cannot #1)
| reference variables defined in main JSP and #2) cannot have any imports. That means that JSP file ehucka is testing on
| is invalid. (?) I need to check JSP spec on this.

I do not understand I use the fragments in this way for a long time 2-3 years. Tomcat compiles the jsps without any
troubles.

I didn't claim they cannot have imports, only that they should not reference objects defined in containing documents (and I am not 100% sure of that now 
either)

Infinite loop mentioned by dkonecny fixed.

Checking in TokenList.java;
/cvs/java/editor/src/org/netbeans/modules/java/editor/semantic/TokenList.java,v  <--  TokenList.java
new revision: 1.4; previous revision: 1.3
done

As for the 50923 - At the time I created the hotfix we never used parser results for unopened files AFAIK. During the
time it has changed and the fix become a problem. Should I fix it, or do you already work on it? I belive there are two
main options:
1) remove the fix - it may cause some regressions, maybe not big, maybe even any (lot a thinks changed in editor from
that time)
2) introduce new method for getting parse results for not edited files - it would disable the synchronization on
EditorSupport.componentActivated().

Created attachment 52275 [details]
proposed fix

I prefer to remove the "fix". Any fixed time delay is wrong as long term solution. I attached the change and would like
to ask you for review. From CVS history of the TagLibParseSupport.java I learned that putting parser task into wait
state had two reasons:

#1) issue 45934 - opening JSP is slow: removing the wait significantly slowed down first time JSP opening. Time dropped
from 200ms to 3000ms. (btw. first time JSP opening in today's build is three times faster than month ago - the best time
I used to got on the same config and same file was around 600ms on average). In order to resolve this I had to change
initial priority of parsing task from Thread.MAX_PRIORITY to Thread.MIN_PRIORITY which solved the problem but I would
like to review this as it may have an impact somewhere else.

#2) issue 50923 - opening folder full of JSPs: should be still fine because we are using dedicated requestProcessor
queue with throughput 10

After this fix Goto source is quick, but consistently wrong - it just opens random sources. But that's the same
behaviour as before this fix so I guess it is just next issue to be resolved. Thanks Marek, David.

Just from a glimpse of the patch I see two problems

1)

public Task prepare() {
-        return parseObject(Thread.MAX_PRIORITY - 1);
+        return parseObject(Thread.MIN_PRIORITY);
     }

AFAIK we want to run the task with a high priority in the case when someone explicitly asks for the parse data. Code
completion is a good example - we need the result ASAP since we are blocked by it.


2) The thread priorities dosn't work well on linuxes, at least before the few years the JDK tends to ignore the
priorities, please verify the jsp opening time on linux, windows is OK (the priorities really does matter).

As for the "15 seconds" bad design pattern - sure, pure wait() looks nicer, it is then easier to find the problem since
users tends to complain about thredlocks, but it was in high resistance, so anything preventing problems helped :-) I
should learn the lesson and always reimplemeted hacks in the next release ...

Thanks,
marek

Re. "broken Goto source" - I filed issue 120864 with simple reproducible test case.

The key problem is in performance improvement of JSP file opening done in issue 45934 - the fact that JSP parser is
always put into wait state and is resumed when JSP file is opened in editor. That's plain hack. JspContextInfo (API)
does not have any Javadoc on parsing methods but any implementation should do just parsing. An implementation does not
know anything about context in which it is called to make any assumptions whether parsing request should be done
immediately or whether it can be paused and wait till something is opened in editor. Only client of the parsing API can
safely make such an assumptions.

Re. "fiddling with thread priorities" - agreed that it is not a good idea. Perhaps possible short term hotfix but
considering linux issues (e.g. JDK issue 6495300) we should just avoid it for now and leave threads in default priority.
(conservative approach)

The proper solution seems to me:
* remove 15sec delay and code which resumes task after jsp was open in editor
* remove fiddling with thread priorities
* do not start parsing thread during document opening - eg. postpone it when document is visible in editor

Marek, do you think it is doable? Do you have an idea of some other solution? I do not have enough expertise in editor
to be able to evaluate this. I changed TagLibParseSupport.getJSPColoringData() to not start parsing and that seems to do
the trick - parsing is started later via SimplifiedJSPServlet. Looking at usage of getJSPColoringData() it seems to be
safe not to start parsing but why was it there in first place then? Can this change break something? Perhaps
JspContextInfo (*public* API, not friend!) should be extended to have method getJSPColoringData(boolean
startParsingThread) and JSPKit would call it with false. Thanks for your opinion Marek, David.

Thanks David for the detailed investigation. I generally agree with your conclustions, I have just a few minor comments:

If we change the TagLibParseSupport.getJSPColoringData() not to start the parsing thread, we need to guarantee that the
parser starts ever. Without doing some extra initialization we just rely on somone else calling
TagLibParseSupport.getCachedParseResult(). If noone start the parsing thread by calling the method the user of
TagLibParseSupport.getJSPColoringData() will never get any reasonable data - the object contains just some defaults, no
events will be fired ever. You may object that there is always the old good SimplifiedJSPServlet calling the
TagLibParseSupport.getCachedParseResult() after the page is opened initializing the parser, but it seems to me to be
similar as the "15 seconds hack". At least if one just wants to read the coloring data without opening the file it
simply wont work.

On the other side if we are the only clients of 

JspContextInfo.getJSPColoringData(Document doc, FileObject fo);

we may assume that noone else will use the method. In such case at least some reasonable documentation should be added.
The same applies for the whole API - it is almost not documented. BTW, the api is formally public, but in fact it is
just friend and IMHO can be treated as such. Another task - make the API friend oficially.

Supposing we agree on the above the only problem is how to ensure that the parsing task is called once the file is
opened. There is autoParse() method in the TLPS, it is now however triggered by document change only, it is called
during the file open (from BaseJspEditorSupport.initialize()). It used to be called even during the file opening, but it
was causing performance problems since the initialize() is called very soon during the file open.

Another problem is that the proposed solution doesn't guarantee that anyone else calls
TagLibParseSupport.getCachedParseResult() during the file openinig. It seems that noone is doing that now, but still it
is another risk. I understand and agree with what you wrote about the assumptions about the usages, but it simply can
happen either accidently or intentionally.

So my proposal is: 
1) remove the changes from fixes of 45934 and 50923 
2) KEEP the original fiddling with thread priorities - it works fine on windows and solaris so why to slow down editing
by the background parsing thread etc.
3) do not start parsing thread during document opening after calling TagLibParseSupport.getJSPColoringData()
4) document the behaviour in the JspContextInfo well.
5) Ensure the TLPS.autoParse() is called & modify its implementation to simple  "return
parseObject(Thread.MIN_PRIORITY);" We need to ensure it is called as late as possible -
BaseJspEditorSupport.componentActivated() seems to be the best place.
---after 6.0---
6) think the entire strategy over again more deeply and possibly reimplement
7) make the API friend

You already done #1 and #3, can you also do #4 and #5? Or should I do that? 

Anyway please attach the patch so I can review.

Thanks Davide for the great job. I am looking forward for your attendance in the #6 process soon.

Agreed. I will look at that later this weekend. Thanks, D.

Created attachment 52481 [details]
patch I'm about to commit

I attached the diff of my fix and also commit it to trunk.

Re. your proposal:

  1) remove the changes from fixes of 45934 and 50923
done

  2) KEEP the original fiddling with thread priorities
have not touched anything

  3) do not start parsing thread during document opening after calling TagLibParseSupport.getJSPColoringData()
done

  4) document the behaviour in the JspContextInfo well.
leaving this one for you after 6.0 - mainly because I do not know enough what methods really do to comment them

  5) Ensure the TLPS.autoParse() is called
done. see my commit - timer is restarted in BaseJspEditorSupport.open() and TagLibParseSupport.autoParse() checks
whether parsing was started at least once

Re. "solution doesn't guarantee that anyone else calls TagLibParseSupport.getCachedParseResult() during the file
openinig" - yes; offending code should be fixed as performance P2; after 6.0 the API documentation should state not to
do that because....

I'm happy to assist in after 6.0 solution.

Fixed in:
Checking in core/src/org/netbeans/modules/web/core/jsploader/BaseJspEditorSupport.java
new revision: 1.65; previous revision: 1.64
Checking in core/src/org/netbeans/modules/web/core/jsploader/TagLibParseSupport.java;
new revision: 1.52; previous revision: 1.51
Checking in jspsyntax/src/org/netbeans/modules/web/core/syntax/JspUtils.java;
new revision: 1.12; previous revision: 1.11

>  5) Ensure the TLPS.autoParse() is called
>done. see my commit - timer is restarted in BaseJspEditorSupport.open() and TagLibParseSupport.autoParse() checks
>whether parsing was started at least once

I am affraid that the solution starting the background parsing thread in ES.open() may still cause the thread being run
during another code performing tasks needed to open the file. Suposing some reasonable redesing of the API after 6.0 and
with respect to the fact that there is no client calling getJSPColoringData() on not opened file I would put the
autoparse call to the EditorSupport.componentOpened() which is called when the editor gets focus (that mean the editor
text should be visible). 

Oops, the BaseJspEditor.componentActivated already contains

((BaseJspEditorSupport)cloneableEditorSupport()).restartTimer(false);

so I would just remove the call from BJES.open()

Btw, I noticed that some old code which doesn't make sense after your changes is still there, shouldn't we better remove
it? e.g.:

 //test whether the parsing task has been cancelled -
 //someone called EditorCookie.close() during the parsing was waiting
 //on openedLock
 if(!parsingTaskCancelled && getWebModule(jspFile) != null) {

Re. "restaring timer in BaseJspEditorSupport.open()" - it is 2sec timer which is restarted so should not impact file
opening. Only done that way so that it works even for files not opened in editor, but I do agree with you that it is
such a minor case that API should just stay that it is no supported and you have to explicitelly call
getCachedParseResult() to get coloring; or something like that.

Re. "code which doesn't make sense" - in my first patch I put there a comment that it still make sense in case when
tasks are queued in our dedicated request processor (throughput 10). But I do not know how relevant it is. Issue 50923
was talking about opening folder full of JSPs, so perhaps in that case - 100 JSPs opened, read and closed - it make
sense to cancel all parsing requests which are not needed anymore.

Talk to you more tomorrow, but feel free to go ahead and just do both your suggestions.

Re:Re. "restaring timer in BaseJspEditorSupport.open()" just briefly - 2 seconds timeout doesn't mean the thread won't
start bofored the editor gets focus. Belive me, on slower machines this happens offten. Before the years I spend quite a
lot of time playing with this problem. This solution was the first one. But, do not worry, I'll do that, maybe also #2.
Thanks for your work on this issue!

verified