Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 344028 - [client][editor] Investigate performance during updatePage
Summary: [client][editor] Investigate performance during updatePage
Status: RESOLVED WONTFIX
Alias: None
Product: Orion
Classification: ECD
Component: Editor (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-27 15:12 EDT by Felipe Heidrich CLA
Modified: 2015-05-05 16:01 EDT (History)
4 users (show)

See Also:


Attachments
pedro's changes (306.84 KB, application/octet-stream)
2011-04-27 15:13 EDT, Felipe Heidrich CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Felipe Heidrich CLA 2011-04-27 15:12:36 EDT
From the mailing list:
"Not all optimizations I used at Firebug's SourceBox can be applied to Orion's editor due to the read-only of SourceBox implementation. But there is room for improvement in Orion's editor and I estimate that we could improve the overall rendering performance by 15% to 30%. Some changes are relatively easy to apply and others would require some harder code refactoring. I divided this message in two parts: 1) Changes that do not require complex code refactoring; and 2) Changes that require more complex code refactoring.

I implemented a prototype in order to test the impact of #1, based on the nightly build N20110425-2000. This prototype will only work on Firefox because I had to hardcode the following browser detection variables and it requires console.log function, but you could easily make it work on other browsers by tweaking these lines and adjusting the log() function if necessary:
    var isPad = false;
    var isIE = 0;
    var isWebkit = 0;

The prototype is attached to this message, and here are the steps to reproduce it:
  a) Open the orion.html page in Firefox
  b) Enable and open Firebug in another window (so the editor covers the whole browser's window)
  c) Press "Test Line Down" buttom
  d) See the time spent to proccess the test at the Console Panel
  e) Press "New _updatePage" buttom (it will replace the _updatePage with the prototype function)
  f) Press "Test Line Down" buttom again
  g) See the time spent to proccess the test at the Console Panel


---------------------------------------------------------------------------------
1) Changes that do not require complex code refactoring
---------------------------------------------------------------------------------

The most expensive thing is reading and writing to the DOM. Not only adding nodes to the DOM tree is expensive but also traversing the DOM tree. Also, reading properties such as element.offsetHeight or element.clientWidth have a considerable impact on performance once it triggers a reflow of the page.

The performance bottleneck is located at the _updatePage() function. The main problem here is that this function is responsible for doing two different things: 1) resizing the editor elements and 2) updating the visible lines. Most of the time we only need to update the lines, so we could improve the performance by 15% to 20% by dividing the _updatePage() into two different functions (the names used here are only illustrative):

1) _resizePage() - this function would only resize the editor elements but would not alter the lines. This function would be called when the editor is resized, but also when the model is changed (when text is added to or removed from the model)

2) _updateLines() - this function would only update the lines and would be called when scrolling the editor, moving the cursor (carret) or changing the selection.

Also, the calculation of the line width and height could be improved. Once the editor uses a fixed-size character, we could calculate the character width and height during the editor initialization. Then the line height would always be the same as the character height, and the line width would be number-of-characters * character-width. This would have a considerable impact because calling the _getLineBoundingClientRect() for each line is very expensive.

Finally, what is the purpose of _overlayDiv? It is only being used in Firefox and I could not find a reason for it. Can we get rid of it? This would save some DOM manipulation.

-------------------------------------------------------------------------------
2) Changes that require more complex code refactoring
-------------------------------------------------------------------------------

The fastest way to generate several DOM nodes at once is using the innerHTML property. Is it possible to refactor the line generation code so all lines are generated as strings and appended to the editor at once using innerHTML? This would improve the rendering performance even more (I estimate an improvement between 10% to 15%).

Instead of iterating over each line using DOM methods (via "child.nextSibling" or "_getLineNext()"), we could have an unique ID for each line and have a list of IDs of lines to be updated. The ID could be something like "editorUniqueID_lineNumber" so we can easily translate from line number to line ID. This way of iterating the lines is being used three times during _updatePage(), when removing lines, when creating lines, and when calculating the maximum length. If we use getElementById(lineID) we would save a lot of DOM access.

I tried to cover all aspects here and the prototype's code has some informative comments, but feel free to ask me questions if something isn't clear in my message and/or code.


my best regards,

Pedro Simonetti."
Comment 1 Felipe Heidrich CLA 2011-04-27 15:13:40 EDT
Created attachment 194196 [details]
pedro's changes
Comment 2 Silenio Quarti CLA 2011-04-27 15:45:33 EDT
It seems that the only change in your prototype that really made any difference was the deletion of the line:

clientDiv.style.width = (0x7FFFF).toString() + "px";

If we comment this line in the original updatePage() function, its time will be equivalent to the new updatePage().

This line is a work around for a problem we found on WebKit. It does not have to be done for Firefox (IE, etc). We released a change to make the line only run for WebKit.

http://git.eclipse.org/c/e4/org.eclipse.orion.client.git/commit/?id=760150154880a9e56293c0caf9ddf9d8d7158201

I am not sure the other changes will improve performance. Could you confirm that this is true on your machine?
Comment 3 Silenio Quarti CLA 2011-04-27 16:00:34 EDT
> Also, the calculation of the line width and height could be improved. Once the
> editor uses a fixed-size character, we could calculate the character width and
> height during the editor initialization. Then the line height would always be
> the same as the character height, and the line width would be
> number-of-characters * character-width. This would have a considerable impact
> because calling the _getLineBoundingClientRect() for each line is very
> expensive.
>
> Finally, what is the purpose of _overlayDiv? It is only being used in Firefox
> and I could not find a reason for it. Can we get rid of it? This would save
> some DOM manipulation.

I do not think the calculation of the line width can be done that way even for monospaced fonts.  I believe it would work only for english characters. It would fail if the text had japanese characters because of font substitution. 

The overlayDiv is necessary to avoid the browser from changing the selection when the user clicks and drags the mouse button in the editor. We used different techniques in other browsers ("selectstart" event for Webkit, setCapture/releaseCapture for IE), but none of them worked for Firefox.
Comment 4 Felipe Heidrich CLA 2011-04-27 16:05:49 EDT
> I do not think the calculation of the line width can be done that way even for
> monospaced fonts.  I believe it would work only for english characters. It
> would fail if the text had japanese characters because of font substitution. 
> 
it would also be wrong for tab expansion, text ranges with bold, combining character, etc.
Comment 5 Felipe Heidrich CLA 2011-04-27 16:13:08 EDT
> Instead of iterating over each line using DOM methods (via "child.nextSibling"
> or "_getLineNext()"), we could have an unique ID for each line and have a list
> of IDs of lines to be updated. 

For iterating over all lines I believe child.nextSibling is faster than getElementById. It depends on the number of children, I published a little test of mine here:
http://jsperf.com/getelementbyid-vs-while/2

One place where using getElementById() can probably help is: editor._getLineNode()

That said, I don't think this code is hurting performance.
Comment 6 John Arthorne CLA 2015-05-05 15:48:47 EDT
Closing as part of a mass clean up of inactive bugs. Please reopen if this problem still occurs or is relevant to you. For more details see:


https://dev.eclipse.org/mhonarc/lists/orion-dev/msg03444.html
Comment 7 John Arthorne CLA 2015-05-05 16:01:55 EDT
Closing as part of a mass clean up of inactive bugs. Please reopen if this problem still occurs or is relevant to you. For more details see:


https://dev.eclipse.org/mhonarc/lists/orion-dev/msg03444.html