Keep row spans in an object pool to reduce garbage collection by reusing DOM nodes #450
Conversation
|
Testing this is a little more involved, putting on hold until the other more important perf PRs land. |
Tyriar
added
work-in-progress
|
This is particularly tricky to perf test. What (I think) I'm seeing is a slight degradation in overall command speed (~5%?), more DOM nodes created (which needs to be investigated) but less JS memory usage/GC. I do think it's a valuable change, it needs more work though. |
|
This is ready to review/merge. Update:
|
|
On performance of unicode heavy apps like Before:
After:
|
src/utils/DomElementObjectPool.ts
Outdated
| this._inUse = {}; | ||
| } | ||
|
|
||
| public acquire(): HTMLElement { |
There was a problem hiding this comment.
Document this method please, with a comment above this line.
src/utils/DomElementObjectPool.ts
Outdated
| return element; | ||
| } | ||
|
|
||
| public release(element: HTMLElement) { |
There was a problem hiding this comment.
Document this method please, with a comment above this line.
src/utils/DomElementObjectPool.ts
Outdated
| this._pool.push(element); | ||
| } | ||
|
|
||
| private _createNew(): HTMLElement { |
There was a problem hiding this comment.
Document this method please, with a comment above this line.
src/utils/DomElementObjectPool.ts
Outdated
| return element; | ||
| } | ||
|
|
||
| private _cleanElement(element: HTMLElement): void { |
There was a problem hiding this comment.
Document this method please, with a comment above this line.
src/xterm.js
Outdated
| @@ -760,7 +760,9 @@ Terminal.loadAddon = function(addon, callback) { | |||
| * character width has been changed. | |||
| */ | |||
| Terminal.prototype.updateCharSizeCSS = function() { | |||
| this.charSizeStyleElement.textContent = '.xterm-wide-char{width:' + (this.charMeasure.width * 2) + 'px;}'; | |||
| this.charSizeStyleElement.textContent = | |||
| '.xterm-wide-char{width:' + (this.charMeasure.width * 2) + 'px;}' + | |||
There was a problem hiding this comment.
Non-blocking: Should we use template literals here as well?
Fixes #449
Fixes #467
This PR adds and utilizes an object pool in
Terminal.refreshso that all the spans and text nodes are not being thrown away sometimes milliseconds later.This introduces one change in behavior in that it wraps plain text in a span, not a text node as there seemed to be issues with adding text nodes only containing
. I think this should have minimal impact on perf though.A good illustration of it is action is the below shot of the DOM after a
for x in {1..500000}; do echo $x; done, this would previously have generated 100-1000 DOM elements (the number is low due to refresh rate limiting and frame skipping) but you can see the IDs which are assigned in sequence are all in the double digits as the nodes are being reused: