Bug 37802

Created attachment 12444 [details]
Diagnostic patch

Created attachment 12445 [details]
Excerpt from log file showing various VN/VC events and the assertion error (gzip format)

Pasting in from an email from Ivan Solemanipour, who's encountered
similar things in the debugger:

"Ok, I'll take your word for it now, until you answer my last question
below, but the performance problems I've had with TTV's are in a
slightly different area. For example ...
	- If I have a node with 5 props and I set them all in sequence each
one will fire a property change and cause a refresh of a row.

	- some of prop changes will cause a resort. If I apply changes to N
nodes I get N resorts. In general it seems like asome sort of
"batching" facility would be useful, at the level of queueing of prop
changes but that's pretty hairy stuff. The next best thing would be to
have a flag to keep the view from refreshing itself until "we're done
adjusting the world". This would be particularly useful in places
where we blow away a lot of nodes and re-create differentones (like
the locals view)."

---------------------

Seems like a related problem.  I have no idea why the resorting is
happening, since I haven't dug below the renderer level of the
TreeTableView yet (nor am I likely to for at least a month, maybe more).

There is a lot of weird cross-translation of events.  Cc'ing Ivan and
Jirka in case there are any insights.

Hmmm, I know that the debugger sorts breakpoints even if the
summary column is set to unsorted. That's because the "natural order"
isn't considered natural enough. 

It stands to reason that if the Name (primary sort key) is changed
that a resort needs to occur?

A vaguely possible solution - probably more of a hack, and might not
even work.  If you could hold the AWT tree lock for the duration of
the batched change operations, I could probably defer all resorts
until after the lock is released, and coalesce any duplicates.  And
holding the lock would defer any repaints as well.  Whether it would
cause some kind of deadlock is a totally different question.

Anyway, I'll attach a slightly weird patch to try to do this.  Let me
know if there's something obviously wrong with the idea - there might be.

Created attachment 12447 [details]
Patch to defer all resorts while AWT tree lock is held and coalesce/reschedule them to EQ

Uh, wouldn't you have to actually post DelayedSorter into EQ or RP or
something? The patch doesn't look like it would do anything.

I'm not sure this is the right approach, anyway... the method calls
from TTV are just wrong, apparently. Calling
VisualizerNode.doChildrenReordered seems to be the root of the evil.
Perhaps this method should be removed (or made private) and
TreeTableView should be rewritten to sort nodes more safely - e.g. by
creating a filter node tree to surround the regular nodes, which
automatically sorts children according to the Nodes API. This would
ensure that VisualizerChildren would get normal node events just like
any other view.

I am making this issue depend on issue #35833 since in that branch all
of this insane garbage is deleted and replaced by a simple
straightforward TreeModel based directly on Node's. Unfortunately I
don't think it is possible to merge that stuff from the branch, since
it depends on Children being updated only in EQ, which is not the case
in the trunk.

A possible workaround is in the tasklist/suggestions module - tasklist
could ensure that all of its node operations occur in EQ (it is a good
idea, even though the Nodes API does not currently require it). I am
guessing that one factor in the bug is that insert/remove requests are
coming from RP (being replanned to EQ by the visualizer layer) but TTV
(running only in EQ I hope) is frequently fooling around with the raw
VisualizerChildren list without going through Children.MUTEX. VC is,
AFAIK, not designed to handle this - it is expecting to be an EQ-only
mirror of an model (Children) which may be changing at any moment on
another thread.

If you have some patch that you believe would work, I can try running
with it for a while to see if it helps.

FWIW I made a trivial patch to tasklist-core.jar that I am running
with, in line with my earlier comments, and so far I have not seen the
error. So maybe that is enough as a workaround, until #35833 can be
finished - i.e. require that anyone using TTV use only EQ for Children
mutation methods (e.g. C.K.setKeys).

Created attachment 12448 [details]
Suggested workaround in tasklist/core

Created attachment 12450 [details]
Better patch

Jesse, you're right - when I rewrote that patch a bit before I
attached it, I left out actually posting the sort event into the EQ.

Not that it's actually necessary to use the AWT tree lock - it could
be any lock.

Look on these issues for more background why the sort go this way:
http://www.netbeans.org/issues/show_bug.cgi?id=31698 and
http://www.netbeans.org/issues/show_bug.cgi?id=32328.
Anyway, the attached patch with coalescing multiple changes looks
useful for me too.

Have not yet tried Tim's revised patch. Anyway the patch to
tasklist-core.jar I attached is working fine for me. So I will open a
separate patch item for this - it would anyway be desirable under #35833.

Tim, do you want me to test your revised patch for a while *without*
the tasklist patch to see if it actually does anything? It doesn't
look to me like it addresses the problem as I understand it, which is
that (1) VN.dCR is inherently unsafe to call since it can be out of
order w.r.t. real NodeReorderEvent's, (2) TTV should not be sorting at
the level of VC anyway since VC is not designed to handle it. The
patch might be useful for other reasons not related to this bug, e.g.
efficiency as Ivan suggested.

Cf. issue #37846 in tasklist.

Marking this P3 - for one thing, a workaround is possible, for
another, until there's time to do a serious rewrite, I'm not going to
spend a lot of time putting still more band-aids and chewing gum on
TreeTableView.

Clearing started status

Workaround in tasklist triggers issue #38244. 

Tim, could you fix till promo-D. AWT workaround causes reponsiveness
problems. Setting keys touches lookup. On lookup there are listening
projects actions. All done synchronously from AWT => UI freeze :-(.

I can't say better than maybe.  I hoped (as you know) to rewrite TTV for promo D, but my 
time has been scheduled for other things.

I intend to branch and start an attempt to integrate contrib/ttv this weekend.  If I make a 
lot of progress I might be able to give you something (delivered as a separate module) that 
will work.  But don't hold your breath - there are other things on my plate.

Petr can you be more specific about the project actions? It should be
fine to be updating the keys in EQ and having actions react to that
synchronously. We have a report of a performance bug in ProxyLookup (I
think) which is under investigation and should be solved. If you have
any other information, please file a separate bug report with details.

The ProxyLookup bug links to my duplicate.

How bad is this bug?  Does it appear in NB standard build?

If it's P2 we can't easily mark the TM to "future".  To release we
have to fix all P1s and most of P2s

My understanding from Jirka is that this is the result of a quick hack to fix some problem 
just before 3.4 or 3.5 was released.  I'm sure resorting the children constantly does 
nothing nice for performance, not to mention that VisualizerNode is not really meant to be 
abused this way.

At the same time, I'd prefer not to invest a lot of time into the existing TTV codebase, 
given that we are rewriting it and the odds that any fix will cause new bugs are quite high.

Fixed - at least, with the tasklist workaround reverted, it is unreproducible.  What I did was 
simply coalesce all resorts and run them later on the EQ, so the underlying Node is at least 
much more likely to be in a reasonable state vis-a-vis the VisualizerChildren.  It is of 
course still possible for some other subsequent change happening on a different thread as 
the queued stuff runs to cause the problem to recur, but much lower probability;  we'll fix 
this properly in the TTV rewrite.

Checking in src/org/openide/explorer/view/TreeTableView.java;
/cvs/openide/src/org/openide/explorer/view/TreeTableView.java,v  <--  
TreeTableView.java
new revision: 1.62; previous revision: 1.61
done

Workaround in tasklist/core removed. Thank you.

Checking in src/org/netbeans/modules/tasklist/core/TaskChildren.java;
new revision: 1.18

Nope, happened to me in 040723, 1.5.0 b58, GTK. Had tasklist window
open, showing all todos in ant/freeform. Clicked on Current File (with
ClasspathPanelTest.java open in the editor). Showed apparently correct
results but threw this exception. Not reproducible.

Created attachment 16425 [details]
Stack trace

Dang!

Well, I'll bang on it some more, but if it's risky, the workaround may have to go back in.  
You're right that the correct way to do this would be filternodes to do the ordering, but 
that would be a pretty massive change to a soon-to-die codebase.

Should really be fixed this time - the problem codepath is gone completely; instead, we 
just pass the Comparator to the VisualizerNode and let it use it to sort its children.  
Heaven knows why it wasn't done this way to begin with.

Checking in src/org/openide/explorer/view/TreeTableView.java;
/cvs/openide/src/org/openide/explorer/view/TreeTableView.java,v  <--  
TreeTableView.java
new revision: 1.63; previous revision: 1.62
done
Checking in src/org/openide/explorer/view/VisualizerChildren.java;
/cvs/openide/src/org/openide/explorer/view/VisualizerChildren.java,v  <--  
VisualizerChildren.java
new revision: 1.15; previous revision: 1.14
done
Checking in src/org/openide/explorer/view/VisualizerEvent.java;
/cvs/openide/src/org/openide/explorer/view/VisualizerEvent.java,v  <--  
VisualizerEvent.java
new revision: 1.9; previous revision: 1.8
done
Checking in src/org/openide/explorer/view/VisualizerNode.java;
/cvs/openide/src/org/openide/explorer/view/VisualizerNode.java,v  <--  
VisualizerNode.java
new revision: 1.42; previous revision: 1.41
done

closed