Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 328225

Summary: [formatter] formatting is slow for big files
Product: [Modeling] TMF Reporter: Knut Wannheden <knut.wannheden>
Component: XtextAssignee: Project Inbox <tmf.xtext-inbox>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: moritz.eysholdt, sebastian.zarnekow
Version: 1.0.1Flags: sebastian.zarnekow: indigo+
Target Milestone: M3   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
patch for PreferenceStoreIndentationInformation
none
updated patch
none
updated patch 2 none

Description Knut Wannheden CLA 2010-10-20 05:33:36 EDT 
Formatting a big file using a formatter implemented as a subclass of AbstractDeclarativeFormatter can take a very long time: Around 10 seconds for a 4000 lines file (this of course also depends on the grammar and actual formatting rules).

I traced the problem back to the FormattingConfigBasedStream$Line#getIndentation() method, where FormattingConfig#getIndentationSpace() is potentially called many times. By default each call will end up in PreferenceStoreIndentationInformation#getIndentString() where two calls to the Eclipse IPreferenceStore are made. These can be expensive.

Changing the PreferenceStoreIndentationInformation to cache the value from the IPreferenceStore and register itself as a listener to the IPreferenceStore (to invalidate the cache) reduces the time spent formatting considerably. I will attach a corresponding patch.
Comment 1 Knut Wannheden CLA 2010-10-20 06:31:29 EDT 
Created attachment 181273 [details]
patch for PreferenceStoreIndentationInformation
Comment 2 Knut Wannheden CLA 2010-10-20 06:37:45 EDT 
Created attachment 181274 [details]
updated patch

Surprising how easy it is to introduce a new bug...
Comment 3 Moritz Eysholdt CLA 2010-10-21 02:54:35 EDT 
Hi Knuth,

thanks for the observation and thanks for the patch. However, rather than adding a property listener to the property store, I'd like the idea to have org.eclipse.xtext.formatting.impl.FormattingConfigBasedStream.Line.getIndentation(int) cache the indentation-value. An instance of FormattingConfigBasedStream lasts exactly for one formatting operation (i.e. resource.save() or CTRL+SHIFT+F) and thereby provides the right context for caching.
Comment 4 Moritz Eysholdt CLA 2010-10-21 02:55:36 EDT 
sry, never mind the "h" ;)
Comment 5 Knut Wannheden CLA 2010-10-21 03:59:47 EDT 
Created attachment 181367 [details]
updated patch 2

Hi Moritz,

Makes sense. I've attached a revised patch.
Comment 6 Sebastian Zarnekow CLA 2010-10-21 04:13:01 EDT 
Hi Knut,

I prefered you former solution as it worked transparently for any possible client of IIndentationInformation. I'm currently discussing this with Moritz.

Regards,
Sebastian
Comment 7 Knut Wannheden CLA 2010-10-21 13:20:36 EDT 
Then how about both? ;-)
Comment 8 Sebastian Zarnekow CLA 2010-10-22 03:31:55 EDT 
(In reply to comment #7)
> Then how about both? ;-)

We came to the conclusion that the PreferenceChangeListener-approach has the advantage that it works transparently for more clients. Please feel free to cache the indentation in the formatter, too, if it improves anything.
Comment 9 Knut Wannheden CLA 2010-10-25 04:15:06 EDT 
PreferenceChangeListener patch applied and pushed to master.