Bug 222161 - org.netbeans.modules.editor.indent.IndentImpl.reformatLock: LowPerformance took 45189 ms.
org.netbeans.modules.editor.indent.IndentImpl.reformatLock: LowPerformance to...
Status: NEW
Product: php
Classification: Unclassified
Component: Editor
7.2
All All
: P3 (vote)
: 7.4
Assigned To: Ondrej Brejla
issues@php
EXCEPTIONS_REPORT perf-profileme
: PERFORMANCE, PLAN
Depends on: 230179
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-15 00:44 UTC by tmannherz
Modified: 2013-05-23 13:30 UTC (History)
6 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
nps snapshot (186.38 KB, application/nps)
2012-11-15 00:44 UTC, tmannherz
Details
Patch (4.34 KB, patch)
2012-11-28 13:41 UTC, Ondrej Brejla
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tmannherz 2012-11-15 00:44:10 UTC
This bug was originally marked as duplicate of bug 183962, that is already resolved. This bug is still valid, so this seems to be another bug, but it might be related.

Build: NetBeans IDE 7.2.1 (Build 201210100934)
VM: Java HotSpot(TM) Client VM, 23.5-b02, Java(TM) SE Runtime Environment, 1.7.0_09-b05
OS: Windows 7

User Comments:
tmannherz: Creating new PHP class file within Git repo.



Maximum slowness yet reported was 45189 ms, average is 45189
Comment 1 tmannherz 2012-11-15 00:44:15 UTC
Created attachment 127826 [details]
nps snapshot
Comment 2 Dusan Balek 2012-11-15 08:55:57 UTC
EDT is blocked by GsfReformatTask trying to acquire the parser lock while nonbreakable OccurrencesFinder is in progress.
Comment 3 Svata Dedic 2012-11-27 21:34:38 UTC
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.
Comment 4 Ondrej Brejla 2012-11-28 12:43:02 UTC
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?
Comment 5 Ondrej Brejla 2012-11-28 12:49:13 UTC
Not sure if I can simply replan the whole OccurrencesFinderImpl.run() to another thread...
Comment 6 Svata Dedic 2012-11-28 12:54:54 UTC
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.
Comment 7 Ondrej Brejla 2012-11-28 12:56:55 UTC
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?
Comment 8 Ondrej Brejla 2012-11-28 13:41:38 UTC
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.
Comment 9 Svata Dedic 2012-11-28 14:11:05 UTC
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.
Comment 10 Ondrej Brejla 2012-11-28 14:48:10 UTC
(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().
Comment 11 Ondrej Brejla 2012-11-29 16:02:29 UTC
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.
Comment 12 Svata Dedic 2012-11-30 14:13:47 UTC
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.
Comment 13 Svata Dedic 2013-05-22 15:03:36 UTC
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 ?
Comment 14 Svata Dedic 2013-05-23 13:30:00 UTC
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.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo