Bug 222161

Created attachment 127826 [details]
nps snapshot

EDT is blocked by GsfReformatTask trying to acquire the parser lock while nonbreakable OccurrencesFinder is in progress.

I doubt this can be solved in CSL. The reformat support in PHP indicates that it needs a parser result in order to operate. So the formatter waits for the result to become available while the parsing is blocked by semantic processing within PHP.

I also don't suppose that PHP is able to speed up processing which blocks other parser tasks, since most of the reported time is spent somewhere in the Lucene index implementation.

Please evaluate from the PHP side ... if might be possible to offload the most exhaustive processing off the parser cycle - even if CSL would need to provide an additional API.

I'm not sure if I understand all problems. If I look into the snapshot I just see what Dusan described. Fromatting can't be done, because parsing lock is hold by OccurencesFinder. Both of them need it, so then I don't know, what should I do more? I'm not sure if I can somehow replan some processing to another thread... Svato?

Not sure if I can simply replan the whole OccurrencesFinderImpl.run() to another thread...

Well I thought that the OccurrencesFinder could be split into two chunks: go through parsing results and the time-consuming one, digging relevant data from indexes. So the 'easier' part would block the parser cycle, while the tough part would be postponed and carried out later, without holding the parser lock. 

It may be bad idea, or even unimplementable (I don't know how exactly your OccurencesParser work). And even if implementable I would need to change CSL SPIs first, since now the php finder simply cannot split anything.

Simple passing whole run() method to RP doesn't work...probably threading
issues. 

Can't it be done in CSL? To override run() from ParserResultTask, and run some
abstractMethod in RP and that abstractMethod will be implemented in clients? So
clients just rewrite their run() method to that abstractMethod?

Created attachment 128513 [details]
Patch

I just made some tries and this patch seems to be working for me. Whole run() method is run by RP. But it seems that getOccurrences() method is called before that exposed Runneble runs, so there has to be an ugly CountDownLatch to handle that. Imho it could be done on CSL side :) What do you think about that Svato? After applying this patch, then Index (QuerySupport.query()) is accessed in new RP thread. Is that ok? Not sure what are the contracts of QuerySupport, there is almost no JavaDoc :) Thanks.

I don't think it will work well; MarkOccurrencesHighlighter calls both OccurencesFinder.run() and getOccurences() sequentially. The whole MarkOccurrencesHighlighter computation is (currently) called by Parser API and the parser lock is held for the length of the MarkOccurrencesHighlighter execution.

Your patch schedules usage of Parser.Result to a RP from the OccurrencesFinderImpl.run(), which is not correct; Parser.Result should be used only from within the parser result task invocation. 

The CSL now calls run (in your patch spins off a computation in a different thread), then calls getOccurrences(), which waits on that different thread - and may wait for the same duration as now while still holding the parser lock since the whole Highlighter executes as a parser thread.

CSL could itself start the processing calling run() synchronously processing the Parser.Result in the OccurrencesFinderImpl, and then spin off a RP task, as shown in the patch, to collect getOccurrences() a little later. The call to getOccurrences() could then happen outside the parser lock (provided that Parser.Result is not accessed from this execution).

Unless we can split usage of Parser.Result (assumes locking by parser lock), and the long parts of the execution (from stacktrace it seems that the index searches took long), it's not really possible to fix the bug.

(In reply to comment #9)
> I don't think it will work well; MarkOccurrencesHighlighter calls both
> OccurencesFinder.run() and getOccurences() sequentially. The whole
> MarkOccurrencesHighlighter computation is (currently) called by Parser API and
> the parser lock is held for the length of the MarkOccurrencesHighlighter
> execution.
> 
Ok.
> Your patch schedules usage of Parser.Result to a RP from the
> OccurrencesFinderImpl.run(), which is not correct; Parser.Result should be used
> only from within the parser result task invocation. 
Ok, I didn't know that either ;)
> 
> The CSL now calls run (in your patch spins off a computation in a different
> thread), then calls getOccurrences(), which waits on that different thread -
> and may wait for the same duration as now while still holding the parser lock
> since the whole Highlighter executes as a parser thread.
> 
Ok, I thought that the parser lock could be released, after my run() finishes (right after RP.post()).
> CSL could itself start the processing calling run() synchronously processing
> the Parser.Result in the OccurrencesFinderImpl, and then spin off a RP task, as
> shown in the patch, to collect getOccurrences() a little later. The call to
> getOccurrences() could then happen outside the parser lock (provided that
> Parser.Result is not accessed from this execution).
> 
As shown in patch, in our case, getOccurrences() just returns a field, computed in run(), so Parser.Result is not accesses in this call.
> Unless we can split usage of Parser.Result (assumes locking by parser lock),
> and the long parts of the execution (from stacktrace it seems that the index
> searches took long), it's not really possible to fix the bug.
Probably I can't imagine now how we can split this functionallity :) Index is accessed just once (I hope) and that call (occurencesSupport.getOccurence()) doesn't need Parser.Result. I just need Parser.Result to get that occurrencesSupport by parserResult.getModel().getOccurrencesSupport().

Svato, in last two snapshots, there is no php class involved, but GSFReformatTask is everywhere. I'm not sure if it should be assigned to PHP/Formatting.

After discussion, we'll try to enhance the SPI in CSL, so that the PHP processing can be split into locked and unlocked (presumably CPU/indexing-intensive) part.
It's not wise to refactor for 7.3, so keeping in CSL PLAN for 7.4

I would also consider to offload TemplateWizard.handleInstantiate() into a non-AWT thread, which would solve UI responsiveness when the processing of a new file is so complex as in the PHP case.

I'll create something like

    /**
     * Marker mixin interface to be implemented on OccurrencesFinder that causes
     * getOccurrences to be called asynchronously, not whithin the parsing task.
     * If implemented, the {@link #getOccurencesAsync} is called to obtain
     * a Future. The Future will be then run in a separate thread with the option
     * to be cancelled if an additional highlight request comes.
     */
    public interface AsyncCapable {
        /**
         * Provides a Future which will compute and produce the coloring and offsets.
         * If the method returns {@code null}, {@link #getOccurrences} will run
         * synchronously as if the interface was not implemented on the Finder.
         * 
         * @return future instance or {@code null}.
         */
        public RunnableFuture<Map<OffsetRange, ColoringAttributes>> getOccurrencesAsync();
    }


to be implemented (optionally) as a mixin inteface on the OccurrencesFinder implementation. This gives CSL an option to:
* schedule the process
* cancel it, if another cursor event is received for the source

Will it work for you ?

OK, I cleared out the exception reports, all but 2 reports were not related to PHP OccurrencesFinder at all.

I actually prototyped the work split on CSL side, and tried to look into PHP's code - but the Parser.Result is so woven into the code (even referenced from the Model) so I really don't know if the time-consuming operation could be split out from the ParserResultTask.

Please first check on the PHP side - I would prefer not to change CSL APIs just to realize that the relevant refactoring cannot be done in PHP :)

There's one additional possible fix for this - the Formatter acquires the parsing lock but does not attempt to suspend the indexing process. See linked issue #230179 for more details.

I just made mark occurrences support in PHP cancellable so it should help, at least a bit, I hope. Moreover, no report from NB 8.1 so far so closing as fixed, let's wait for reopening.

Thanks.

http://hg.netbeans.org/web-main/rev/09bb1760c6be