View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0013268||Lazarus||Other||public||2009-03-03 06:35||2011-05-25 23:24|
|Reporter||Flávio Etrusco||Assigned To||Martin Friebe|
|Product Version||0.9.26.1 (SVN)||Product Build|
|Target Version||0.9.30||Fixed in Version||0.9.29 (SVN)|
|Summary||0013268: 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 Information||Currently, in ScanFrom:|
Range := Highlighter.GetRange;
Range[n] := Highlighter.GetRange();
I guess it's obvious from this excerpt that Range 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.
|Tags||No tags attached.|
|Fixed in Revision||25320|
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.
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?
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...
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.
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.)
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
|2009-03-03 06:35||Flávio Etrusco||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||Note Added: 0025912|
|2009-03-04 00:51||Flávio Etrusco||Note Edited: 0025912|
|2009-03-04 01:25||Martin Friebe||Note Added: 0025916|
|2009-03-04 01:44||Flávio Etrusco||Note Added: 0025917|
|2009-03-04 01:47||Flávio Etrusco||Note Edited: 0025917|
|2009-03-04 01:47||Flávio Etrusco||Note Edited: 0025917|
|2009-03-04 01:48||Flávio Etrusco||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||Status||resolved => closed|