Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance with multiple documents by up to 100x #24

Merged

Conversation

bmeurer
Copy link
Contributor

@bmeurer bmeurer commented Feb 1, 2019

Previously this exported a single function undom(), which had all the
classes and functions inside, and would create new classes (including
new prototypes, constructors, etc. for those classes) everytime you
create a new document, which is not very efficient to say the least.
So the first step to improve the performance here was to move all the
classes and functions out of the undom() function (which I took the
liberty to rename to createDocument() now, as that seems to make more
sense to me).

The next step was to move the methods and accessors that were slapped
onto the Document and Element instances to the respective prototypes
instead. Specifically we avoid installing the 'cssText' and 'className'
accessors in the constructor, and instead have these accessors installed
once on the Element.prototype, which gave a huge performance boost.

The execution time of the following micro-benchmark, which creates lot's
of documents and adds a single HTMLUnknownElement to it, goes
from around 6690ms to 67ms, which is a solid 100x performance
improvement.

const createDocument = require('undom');

function render() {
  const d = createDocument();
  d.body.appendChild(d.createElement('app-root'));
  return d;
}

for (let i = 0; i < 1e2; ++i) render();

console.time('timer');
for (let i = 0; i < 1e5; ++i) render();
console.timeEnd('timer');

Note that this is a breaking change in the sense that prototypes will be
shared by multiple documents. But this seems to be consistent with
what other DOM libraries like jsdom and domino do.

Previously this exported a single function `undom()`, which had all the
classes and functions inside, and would create new classes (including
new prototypes, constructors, etc. for those classes) everytime you
create a new document, which is not very efficient to say the least.
So the first step to improve the performance here was to move all the
classes and functions out of the `undom()` function (which I took the
liberty to rename to `createDocument()` now, as that seems to make more
sense to me).

The next step was to move the methods and accessors that were slapped
onto the `Document` and `Element` instances to the respective prototypes
instead. Specifically we avoid installing the 'cssText' and 'className'
accessors in the constructor, and instead have these accessors installed
once on the `Element.prototype`, which gave a huge performance boost.

The execution time of the following micro-benchmark, which creates lot's
of documents and adds a single HTMLUnknownElement <app-root> to it, goes
from around 6690ms to 67ms, which is a solid **100x** performance
improvement.

```js
const createDocument = require('undom');

function render() {
  const d = createDocument();
  d.body.appendChild(d.createElement('app-root'));
  return d;
}

for (let i = 0; i < 1e2; ++i) render();

console.time('timer');
for (let i = 0; i < 1e5; ++i) render();
console.timeEnd('timer');
```

Note that this is a breaking change in the sense that prototypes will be
shared by multiple documents. But this seems to be consistent with
what other DOM libraries like jsdom and domino do.
@bmeurer bmeurer force-pushed the perf_GlobalClasses_BetterUseOfPrototypes branch from e0e8411 to fe5f181 Compare February 1, 2019 20:10
@developit
Copy link
Owner

As discussed sidechannel, this means undom's existing promise of avoiding shared prototypes between documents. However, for the performance payoff, that's pretty workable.

Since jsdom and domino both use shared prototypes like this PR, I think the precedent is set and we can merge.

@developit developit merged commit 9b3c97f into developit:master Feb 1, 2019
@developit developit mentioned this pull request Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants