Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 328225 - [formatter] formatting is slow for big files
Summary: [formatter] formatting is slow for big files
Status: CLOSED FIXED
Alias: None
Product: TMF
Classification: Modeling
Component: Xtext (show other bugs)
Version: 1.0.1   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: M3   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-20 05:33 EDT by Knut Wannheden CLA
Modified: 2010-10-25 04:15 EDT (History)
2 users (show)

See Also:
sebastian.zarnekow: indigo+

Attachments
patch for PreferenceStoreIndentationInformation (2.57 KB, patch)
2010-10-20 06:31 EDT, Knut Wannheden CLA 		no flags Details | Diff
updated patch (2.55 KB, patch)
2010-10-20 06:37 EDT, Knut Wannheden CLA 		no flags Details | Diff
updated patch 2 (1.39 KB, patch)
2010-10-21 03:59 EDT, Knut Wannheden CLA 		no flags Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.