Keep row spans in an object pool to reduce garbage collection by reusing DOM nodes#450
Keep row spans in an object pool to reduce garbage collection by reusing DOM nodes#450Tyriar merged 18 commits intoxtermjs:masterfrom
Conversation
|
Testing this is a little more involved, putting on hold until the other more important perf PRs land. |
|
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:
|
parisk
left a comment
There was a problem hiding this comment.
Great job done here. It was one of the most enjoyable Pull Requests to read it's code.
The only thing I would require is documenting all methods of the DOM Element Object Pool, as we might get back to it in the future.
After this, it's ready to merge 👍 .
| 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.
| this._pool.push(element); | ||
| } | ||
|
|
||
| private _createNew(): HTMLElement { |
There was a problem hiding this comment.
Document this method please, with a comment above this line.
| 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
| 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: