When using the properties view the editor becomes blocked till the analysis is finished, for each keystroke.

Without the properties view the editor does not get blocked and there gets a big list of “Analysing updates to …”. Which obviously is a waste of computation (because its doing analysis on outdated ASTs) but at least doesn’t block the editor.

How to reproduce:

  1. create a language in which the analysis of a 200 line file takes more than a second.
  2. compare the behaviour with and without properties view: editor-properties

Task: make the properties view not block the editor.

Submitted by Daco Harkes on 27 March 2014 at 15:11

On 27 March 2014 at 15:15 Gabriël Konat commented:

My hunch is that the property view does not request or wait for the analyzed AST (because this is not possible in the current Spoofax architecture), but probably uses the getAnalyzedAst function to get an analyzed AST, which will (re)trigger analysis. This analysis probably then gets executed in the UI thread, which blocks the UI from rendering.

On 27 March 2014 at 17:00 Gabriël Konat tagged @oskarvanrest

On 27 March 2014 at 18:43 Oskar van Rest commented:

The properties view uses getCurrentAnalyzedAst which doesn’t trigger a re-analysis.
I think the problem is that things are not executed in parallel. It seems like the properties view transformation ends up in the same queue as the analysis, so if it’s currently analyzing and you change the selection in the editor (by typing or clicking), the editor blocks until analysis is finished.
You can see this when you change the selection within 500ms after editing: this doesn’t block the editor because the analysis is not yet queued. But if you click at let’s say 700 ms after the edit, the editor blocks.

Fixing this will requires some changes in the StrategoObserver. I had a quick look at the code and I think that because of getLock().lock(); everything gets executed sequentially. Probably something will break when you remove this lock, but I’ll have a try and see what happens.

On 27 March 2014 at 19:10 Gabriël Konat commented:

Ah, good, do you somehow wait for the analyzed AST then? And yes, Stratego execution is sequential, and removing that lock will most likely break stuff. But shouldn’t both the regular analysis, and property view transformation be executed in a background thread, and feed back information if this is done? Or is this not how it works?

On 27 March 2014 at 19:41 Oskar van Rest commented:

Fixed with https://github.com/metaborg/spoofax/commit/f84a98c30c4d2549d6d859e917b4cf36c3b9c40f
I removed the call to lock() from getCurrentAnalyzedAst and from the properties view service. It wasn’t necessary to remove it from the StrategoObserver.

Now, the properties view is blank while analyzing, which I think is desired behavior.

On 27 March 2014 at 19:41 Oskar van Rest closed this issue.

On 27 March 2014 at 20:29 Gabriël Konat commented:

I think this introduces a race condition where you try to get the AST while it is being set in the StrategoObserver. If calls to resultingAsts.get and resultingAsts.put happen concurrently, things will go wrong. Do you think they can happen concurrently? If so, you need to change the observer to either use a concurrent hash map to store the resulting AST’s, or synchronize access to it.

On 27 March 2014 at 21:11 Oskar van Rest commented:

I didn’t see your other comment earlier. But I think you’re right that it should be executed in the background. Also because everything feels a little bit slower now because of the extra transformation when you change the selection. Let me see if I can make Eclipse do it that way. I’ll undo my earlier changes because I think you’re right that multiple threads can access the map concurrently.

On 27 March 2014 at 21:12 Oskar van Rest reopened this issue.

On 27 March 2014 at 23:00 Oskar van Rest commented:

Things are now properly executed in the background with an Eclipse Job.

On 27 March 2014 at 23:00 Oskar van Rest closed this issue.

On 28 March 2014 at 09:12 Gabriël Konat commented:

Great, thanks! The lock should prevent a race condition.

Log in to post comments