View Issue Details

IDProjectCategoryView StatusLast Update
0013268LazarusOtherpublic2011-05-25 23:24
ReporterFlávio Etrusco (notifications not working)Assigned ToMartin Friebe 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version0.9.26.1 (SVN)Product Build 
Target Version0.9.30Fixed in Version0.9.29 (SVN) 
Summary0013268: SynEdit parses every line twice on load or paste
Description(This is a performance bug; someone might prefer to change it to "tweak" or "feature"...)
There was a bug in the "official" SynEdit that's still (AFAICS) unfixed in Lazarus.
The Range information being stored in the StringList is the range at the start of the associated line (or the end of the previous line), but the range at the end of the associated line should be stored instead.

This makes the whole file be parsed twice on best case (highlighter disabled during file load), and caused (or used to, in official SynEdit) a recursive/exponential parse bug during paste or file load with highlighter enabled.
Additional InformationCurrently, in ScanFrom:

Highlighter.ResetRange;
Range[0] := Highlighter.GetRange;
...
Highlighter.SetRange(Range[n -1]);
Highlighter.NextToEol;
Range[n] := Highlighter.GetRange();

I guess it's obvious from this excerpt that Range[0] is currently useless.
And the lack of (stored) range info for the end of the last line will cause the performance bugs mentioned, because we'll need to continually reparse the last line.
TagsNo tags attached.
Fixed in Revision25320
LazTarget1.0
Widgetset
Attached Files

Activities

Martin Friebe

2009-03-03 15:31

manager   ~0025909

Sorry, I can't see where storing the state of "end of rev line"/"start of current line" triggers the described behaviour.
I can not see any place in Synedit where the end of the last line is ever needed.

Highlighter.NextToEol;
is called once per line, it would still be called once per line, if the result was stored on a different/the same Line in Lines[]

* In Paint:
Paint needs to have *every* state that occurs in every line. Since only states at line boundaries (never mind which one) are stored, Paint() must allows rescan any line it paints. While Rescanning it can intercept the State at each token inside the Line.
But that has nothing to do with the subject ("double parse on load or paste)

* In ScanFrom:
- I checked loading a file. The lines where scanned once only
- I checked Paste form clipboard.
There are indeed 2 Scans. But this is independent which boundary would be stored. Since the bug is in the Selection-Insert code: It inserts empty lines first, scans the empty lines, put the content in, scan the content.
[ The loss is very small, since scanning empty lines is quick ]

A similar Bug may have been present in the past when loading a text file, and may have triggered many Scans of partly loaded data. But that seems to be fixed (There has been some Locking been introduced, to reduce overhead)

Whenever one or more lines are scanned (because they were changed/inserted) SynEdit needs to determine that the Ranges are in sync with previous data. This requires scanning of one or two unchanged lines. This is independent of which boundary-state is stored.

--------
In conclusion (unless further feedback/examples) are provided:
- I see no problem with storing the range-state of the start of line (as current) => This is needed for Paint()
- I see a Bug in Paste or Insert (Scanning of empty lines)

I am setting this to feedback, please answer the following:

- can this bug be reduced to the described double scan on Paste?
- If not, can you provide examples where lines are scanned without need?
   Set a breakpoint, and check how often Scan is called for each line,
   provide info what file to load,
   which actions to take (edit, paste, insert, etc...),
   and give a list of all calls to Scan that where made?



Flávio Etrusco (notifications not working)

2009-03-03 23:31

developer   ~0025912

Last edited: 2009-03-04 00:51

I didn't do any debugging yet, but took a better look at the code and I still think the problem is there. But indeed there doesn't seem to be a problem when the Highlighter is unset during load. But I was not talking about "parsing" the empty line; this is indeed insignificant.

(I don't remember all the details and I guess I'm confusing some things, I was just browsing the code to get a notion on the current state of SynEdit in Lazarus because I hope I'll have some free time soon (and the mood) to work on porting some additions and some of my code to Lazarus and stumbled on this.)

Since you don't have the Range Info af the EOL of the last line, every time you append a line (e.g. you load a file, or paste at EOF) with TSynEdit.Highlighter set you'll have to re-parse the previous last line.
TBH there are other ways to avoid this, like hacking the ScanFrom while-loop to avoid parsing the last line in the editor, but...

Martin Friebe

2009-03-04 01:25

manager   ~0025916

Ok, I see. But the missing Range from the very end, simply means that you have to parse one (and just oneline, and only once extra, if you append.

If you load a file, it should load all lines, and the do a complete scan of all lines at the end of this.
This is at least what happens in the IDE.

I have to check what happens if you use Lines.LoadFromFile, it may differ there. However if it does, the fix should be to load all lines, and then do the Scan.

If you use SynEdit in your own application, and use Lines.Add you should call Lines.BeginUpdate first. I do need to check and maybe fix, that BeginUpdate will also defer the Scanning.

The very long time goal is to defer all scanning, until the result is actually used. But that's far future.

Flávio Etrusco (notifications not working)

2009-03-04 01:44

developer   ~0025917

Last edited: 2009-03-04 01:48

LoadFromFile comes from TStrings so there isn't much the StringList can do besides changing the semantics of the 'senrCountChanged' notification and don't trigger it inside 'Add' if UpdateCount <> 0 or something...
(Without the CodeFolding stuff, deferring the scan is pretty trivial BTW.)

Martin Friebe

2010-05-12 02:31

manager   ~0037464

Ranges have now been for a long while representing the end of line.

LoadFromFile should be fixed, since proper locking is now applied => one scan at the end of loading

Please close if ok

Issue History

Date Modified Username Field Change
2009-03-03 06:35 Flávio Etrusco (notifications not working) New Issue
2009-03-03 08:01 Paul Ishenin Status new => assigned
2009-03-03 08:01 Paul Ishenin Assigned To => Martin Friebe
2009-03-03 08:02 Paul Ishenin LazTarget => 1.0
2009-03-03 08:02 Paul Ishenin Target Version => 1.0.0
2009-03-03 15:31 Martin Friebe Note Added: 0025909
2009-03-03 15:31 Martin Friebe Status assigned => feedback
2009-03-03 23:31 Flávio Etrusco (notifications not working) Note Added: 0025912
2009-03-04 00:51 Flávio Etrusco (notifications not working) Note Edited: 0025912
2009-03-04 01:25 Martin Friebe Note Added: 0025916
2009-03-04 01:44 Flávio Etrusco (notifications not working) Note Added: 0025917
2009-03-04 01:47 Flávio Etrusco (notifications not working) Note Edited: 0025917
2009-03-04 01:47 Flávio Etrusco (notifications not working) Note Edited: 0025917
2009-03-04 01:48 Flávio Etrusco (notifications not working) Note Edited: 0025917
2009-03-07 01:09 Martin Friebe Status feedback => assigned
2010-05-12 02:31 Martin Friebe Fixed in Revision => 25320
2010-05-12 02:31 Martin Friebe Status assigned => resolved
2010-05-12 02:31 Martin Friebe Fixed in Version => 0.9.29 (SVN)
2010-05-12 02:31 Martin Friebe Resolution open => fixed
2010-05-12 02:31 Martin Friebe Note Added: 0037464
2010-05-12 02:31 Martin Friebe Target Version 1.0.0 => 0.9.30
2011-05-25 23:24 Flávio Etrusco (notifications not working) Status resolved => closed