Performance issues

[fabiospampinato]

I've been using turndown in enex-dump for converting Evernote's notes to markdown. I've tried to optimize it's performance, but the main bottleneck comes from turndown, here you can check the actual function I'm using, I've tried removing all my custom regexes and extra rules and they barely make any difference.

Part, if not most, of the bloat I think comes from jsdom, which turndown imports. Just because of jsdom I'm seeing ~10MB of dependencies getting installed, which is definitely not good for my use case where I'm embeddeding enex-dump in an Electron application. Also jsdom provides an implementation of the DOM, which I'm not sure a project like turndown really needs, shouldn't a fast xml parser like fast-xml-parser get the job done with significantly less bloat?

Thanks.

[domchristie]

One of the main benefits of using the DOM is that you get HTML parsing for free in browsers:

https://github.com/domchristie/turndown/blob/e2c64f121a170ad04363c7da3bbf978ebcde5f7f/src/html-parser.js#L41-L47

We also use methods defined on DOM nodes for querying. This makes it very easy for developers to extend the rules because the DOM Node/Element interface is familiar. Of course we could re-implement all these methods if we were to parse HTML as JSON, but that seems unnecessary given the above, and we'd lose the familiarity.

I'm not very familiar with Electron, so this could be wrong, but if it effectively just renders HTML to a webview, then it should be possible to use the built in parser, rather than importing JSDOM. For this you should import the browser version from lib/turndown.browser.es.js.

Hope that helps.

[fabiospampinato]

I don't understand the title change, enex-dump is a standalone application and has nothing to do with Electron, turndown is not bloated and slow only when used in Electron applications.

I get your point of view, after all the maintainers are the ones putting the actual work into this, and the project should be something they feel comfortable with. I just wanted to express a bit of frustration since the cause of slowness/bloat for my use case comes from this library.

I'm not very familiar with Electron, so this could be wrong, but if it effectively just renders HTML to a webview, then it should be possible to use the built in parser, rather than importing JSDOM. For this you should import the browser version from lib/turndown.browser.es.js.

Interesting, maybe I could instruct webpack to load the es version for browsers instead.

The problem with this is that, if I understand this correctly, one should be using turndown in a renderer process (the one that actually renders the HTML) to take advantage of this, but it's generally better to do the heavy work on a separate (non-renderer, I guess) process so that the UI feels responsive at all times.

Thanks!

[domchristie]

I don't understand the title change, enex-dump is a standalone application and has nothing to do with Electron, turndown is not bloated and slow only when used in Electron applications.

Sorry if I misunderstood. You mentioned your use case was "embeddeding enex-dump in an Electron application" so I thought that could be where the problem lies. I carried out some brief performance tests after launching v4, and it seemed to be significantly faster than other similar libraries. These tests were performed using the CommonJS build on a Node.js server. As I mentioned, I think the underlying issue is that you're using the non-browser build, and therefore embedding JSDOM within a browser, which is not its intended use :/ We may need to look into updating a few package.json fields in order to prevent this happening so much.

I get your point of view, after all the maintainers are the ones putting the actual work into this, and the project should be something they feel comfortable with. I just wanted to express a bit of frustration since the cause of slowness/bloat for my use case comes from this library.

I'm open to pull requests and feedback. You are obviously more than welcome to explore the source code, replace JSDOM with a parser of your choosing, or suggest improvements. However, the issues aren't really a place to vent frustration in this way.

Interesting, maybe I could instruct webpack to load the es version for browsers instead.

👍

it's generally better to do the heavy work on a separate (non-renderer, I guess) process

Technically true, but I'd recommend trying it and seeing how it feels. If you're converting a significant amount of HTML then you may want to perform this on the server.

[fabiospampinato]

Sorry if I misunderstood. You mentioned your use case was "embeddeding enex-dump in an Electron application" so I thought that could be where the problem lies.

My bad, I should have said more explicitly that I basically have 2 use cases for turndown: 1) using it in enex-dump 2) using it indirectly in an Electron app that uses enex-dump.

I carried out some brief performance tests after launching v4, and it seemed to be significantly faster than other similar libraries.

I don't doubt that, but in my tests turndown needs about 80ms to process a particular html string, which I can convert to json in about 2ms with fast-xml-parser. I'm kind of comparing apples to oranges here, but should converting the document to markdown take ~40x as much as parsing it? Maybe not, I think there's quite some room for improvement here.

As I mentioned, I think the underlying issue is that you're using the non-browser build, and therefore embedding JSDOM within a browser, which is not its intended use :/

I think we should consider my 2 use cases separately for this:

• Standalone: I don't think the ~10mb of dependencies matter much in this scenario, the bad performance is still there though.

• Embedded in Electron: ~10mb of extra dependencies are definitely important in this scenario, and if turndown is slow because it isn't using the native parser it's a problem also. I don't think I'm doing anything "unintended" here though, just importing turndown and using it.

However, the issues aren't really a place to vent frustration in this way.

I'm sorry, but I've opened this issue because turndown requires ~10mb of dependencies and because I think it's not very performant, aren't these good enough reasons for opening an issue? The fact that this frustrates me is irrelevant.

Technically true, but I'd recommend trying it and seeing how it feels. If you're converting a significant amount of HTML then you may want to perform this on the server.

The UI blocks for multiple minutes when converting a large enough number of html strings (100s to 1000s of html strings), on the renderer process. I'll definitely try to run this on another process, hoping to have access to the browser's native parser there.

I'm open to pull requests and feedback. You are obviously more than welcome to explore the source code, replace JSDOM with a parser of your choosing, or suggest improvements.

Unfortunately I don't have the time to make major changes (switching from a full-blown DOM implementation to an xml parser) to this unfamiliar codebase, but I think there might be a few things I can help with:

1. I have to check if within Electron turndown is still using jsdom, if that's the case it should be fixed.
2. jsdom should be loaded conditionally, in such a way that it's import can be removed entirely via dead code elimination.

[domchristie]

I've opened this issue because turndown requires ~10mb of dependencies and because I think it's not very performant, aren't these good enough reasons for opening an issue? The fact that this frustrates me is irrelevant.

Opening an issue to discuss performance issues is fine. Stating that "Turndown is slow and bloated" probably isn't the best way to start one.

I try as much as possible to reduce the number of dependencies; dealing with frequent updates, incompatibilities and conflicts is never fun! It's why Turndown aims to use the simplest set up possible: virtually no transpilation—just ES6 modules compiled with Rollup using npm scripts. JSDOM was chosen early on because it is reliable and well maintained. It offers a solid implementation of the DOM allowing for Turndown rules to use familiar node/element properties, and for them to be shared across different environments: Node and browser. If there is a library that offers similar levels of reliability, with a nice API, that is well maintained, then I'd happily switch to it.

I don't think I'm doing anything "unintended" here though, just importing turndown and using it.

This is a key point. In a Node.js environment, the a node version (with JSDOM) will probably be used. For building JS for electron apps, this is obviously not what we want. There is some discussion around this issue (https://github.com/defunctzombie/package-browser-field-spec) but I've not looked into its status.

One thing to note re: enex-dump, you are using an old version of Turndown. Turndown v5 updated the escaping to be more aggressive, but almost certainly more efficient.

[domchristie]

jsdom should be loaded conditionally, in such a way that it's import can be removed entirely via dead code elimination.

I believe this is already the case:

https://github.com/domchristie/turndown/blob/e2c64f121a170ad04363c7da3bbf978ebcde5f7f/src/html-parser.js#L27-L56

process.browser determines whether JSDOM is required or not. This is set in config: https://github.com/domchristie/turndown/tree/master/config

[fabiospampinato]

Opening an issue to discuss performance issues is fine. Stating that "Turndown is slow and bloated" probably isn't the best way to start one.

That was just a descriptive short title for the content of my message, I didn't mean to offend anyone with it, sorry if it did.

If there is a library that offers similar levels of reliability, with a nice API, that is well maintained, then I'd happily switch to it.

If you're looking for another DOM implementation I'm not sure there are significantly better ones. My argument is that using a XML parser instead would be better for performance, perhaps at the cost of (slightly?) larger bundles for browsers.

One thing to note re: enex-dump, you are using an old version of Turndown. Turndown v5 updated the escaping to be more aggressive, but almost certainly more efficient.

Cool, I didn't realize another version was out. I just benchmarked it and it seems the performance improvement is about 0.05% for me.

jsdom should be loaded conditionally, in such a way that it's import can be removed entirely via dead code elimination. I believe this is already the case:

Kind of, you're completely omitting the import from the bundle, but if the right bundle isn't loaded (which seems to be the case for me) that doesn't matter. Maybe the browser property in package.json can solve that, I'm not very familiar with it 🤔

[domchristie]

Kind of, you're completely omitting the import from the bundle, but if the right bundle isn't loaded (which seems to be the case for me) that doesn't matter

Yes, so I suppose the solution is to ensure the correct version is loaded. The possible versions should probably be documented to help this, so FWIW:

For Node.js environments (JSDOM will be required):

• CommonJS: lib/turndown.cjs.js (for require)
• ES: lib/turndown.es.js (for import)
• UMD: lib/turndown.umd.js (not sure if this will be used much—it's mainly there for AMD/completeness)

For browser environments (JSDOM will not be required):

• CommonJS: lib/turndown.browser.cjs.js (for Browserify)
• ES: lib/turndown.browser.es.js (for ES modules)
• IIFE: dist/turndown.js (globals)
• UMD: 'lib/turndown.browser.umd.js (for AMD)

[fabiospampinato]

Those are a lot of bundles.

I did some tests and:

• I couldn't find a way of instructing webpack to load the proper file, the following configuration changes nothing for me (the $ at the end is supposed to tell webpack "match only this and not turndown/foo for instance, but I also tried omitting it, same thing):

  resolve: {
    alias: {
      turndown$: require ( 'path' ).join ( __dirname, 'node_modules', 'turndown', 'lib', 'turndown.browser.cjs.js' )
    }
  }

• If I patch package.json manually and force it to load the right file then the performance difference isn't really noticeable, so I guess jsdom's implementation of the DOM isn't significantly slower than chrome's, which would be quite surprising to me, or the cause of the slowness is elsewhere in the codebase.

• By default the packaged electron app weighs 162.6mb (58.4mb zipped), if I manually remove the jsdom dependency from package.json then it weighs 154.6mb (56.9mb zipped). Notice how by making the main field point to the right file jsdom is still included in the bundled, I had to manually remove the dependency from package.json. Maybe webpack expects a "sideEffects": false in package.json, or it just doesn't perform this optimization for non-es2015 imports/exports, I'm not sure, here there are some details.

I see turndown already has a browser property in its package.json:

https://github.com/domchristie/turndown/blob/e2c64f121a170ad04363c7da3bbf978ebcde5f7f/package.json#L9-L11

which if my understanding is correct in this particular case should instruct the bundler not to include "jsdom" along with the bundle when bundling for the browser. But I'm not sure how webpack handles this, maybe it uses it if the target is web but not if it is electron-renderer.

Both turndown and jsdom weren't considered external dependencies during my tests.

I'll try to profile turndown and check if there's any low hanging fruit with regard to performance.

[domchristie]

Thanks for taking the time to investigate this further!

Those are a lot of bundles.

The state of JS 😩

Would it be possible to do something like the following in your source:

require('turndown/lib/turndown.browser.es.js')

Perhaps something like this approach might be worth looking into: https://blog.patrickhulce.com/2016/09/15/exposing-multiple-exports-in-a-single-npm-package/?

If I patch package.json manually and force it to load the right file then the performance difference isn't really noticeable, so I guess jsdom's implementation of the DOM isn't significantly slower than chrome's, which would be quite surprising to me, or the cause of the slowness is elsewhere in the codebase.

Interesting. Previous performance issues were due to slow-running regular expressions (https://github.com/domchristie/turndown/issues/188#issuecomment-332216019). These issues should have been resolved by now, but it might be a good starting point?

[fabiospampinato]

Would it be possible to do something like the following in your source: require('turndown/lib/turndown.browser.es.js')

enex-dump is supposed to also work on node environments, so I can't do that. The Electron app that imports enex-dump has no access to that import (I mean unless I manually patch the dependency, which I'd rather not do) so I can't do that there either.

Perhaps something like this approach might be worth looking into: https://blog.patrickhulce.com/2016/09/15/exposing-multiple-exports-in-a-single-npm-package/?

This could be a solution, but I'd prefer to have just 1 export from enex-dump.

Interesting. Previous performance issues were due to slow-running regular expressions (#188 (comment)). These issues should have been resolved by now, but it might be a good starting point?

Regexes may be the bottleneck, I think I'll just profile turndown with the devtools and see what I get. I'll report back as soon as I have some numbers, probably early next week.

[Vaelatern]

Just notable that there are 96 dependencies including jsdom, including a package for working with ssh keys. Would a PR to slim down jsdom as a new package, or as an internal dependency, be accepted?

[fabiospampinato]

@domchristie I've quickly benchmarked turndown and I couldn't find any singular function responsible for the slowness, but it seems that garbage collection can be significant (maybe using a DOM implementation is largely responsible for this) and on my machine just requiring turndown (require ( 'turndown' )) takes half a second, I think this might be because lots of files get read from the disk and have to be parsed.

IMHO the less dependencies the better, my guess is that ditching jsdom all together will give the most performance improvements.

What do you think?

[domchristie]

Would a PR to slim down jsdom as a new package, or as an internal dependency, be accepted?

ditching jsdom all together will give the most performance improvements.

These are interesting ideas. My only concern is that it might be tricky to find a package that is as reliable and well maintained as jsdom, and I'd rather not have to maintain it as an internal dependency.

I'm beginning to think that the best approach might be to implement a Bring Your Own Parser for Node.js environments (as suggested in https://github.com/domchristie/turndown/issues/97). In the meantime obviously feel free to fork and experiment with other parsers.

[domchristie]

I'm beginning to think that the best approach might be to implement a Bring Your Own Parser for Node.js environments

I should add that this is sort of already the case, as Turndown accepts DOM elements as input (as well as strings). So if you're not content with the parsing performance of JSDOM, you could parse the HTML yourself and pass in a valid DOM element into turndown. However this does not solve the slowness of installing and requiring JSDOM. I'm wondering if the HTML parser could be lazily loaded and memoized if required. This would mean that calling turndown for the first time be slow, but it might be favourable?

[fabiospampinato]

@domchristie I'm not familiar with turndown's internals, but I guess if turndowns receives a dom node rather than a string then maybe there's no need to require jsdom at all.

But I think this might just move the problem somewhere else, as now the user would have to parse the string into a dom himself, and if he uses jsdom we aren't achieving anything in terms of performance, and if he doesn't use jsdom what will he use? I'm not familiar with jsdom's internals either, but there's a reason if it's quite popular, and I think its maintainers would also prefer to have a 100kb jsdom rather than a multi-megabytes one, but maybe providing a dom implementation is just something you can't do in 100kb.

IMHO the best option here might be to switch to a fast xml parser instead.

[domchristie]

Yes, technically JSDOM is only required in node.js environments when the input to turndown is a string. In practice, the current implementation requires it by default so that it can be readily used when turndown is called; after all, most developers will just have an HTML string they want to convert, rather than a DOM tree.

But I think this might just move the problem somewhere else, as now the user would have to parse the string into a dom himself, and if he uses jsdom we aren't achieving anything in terms of performance, and if he doesn't use jsdom what will he use?

This highlights the predicament well. The library doesn't have to accept string inputs, but doing so makes it very convenient for developers. I've chosen JSDOM for its accuracy, stability, and its standardised output that is compatible in other environments. The trade-off is that it might be a bit slow when either starting up, or when converting large amounts of text. It sort of comes down to fast, good, or cheap—choose two, where "fast" is performance, and "cheap" is refers to my time :D.

So to summarise, my approach has been to accept HTML strings as input to make it easy-to-use, but with the option of using your own parser.

---

I have been looking at using parse5, which I believe JSDOM uses behind the scenes. parse5-htmlparser2-tree-adapter outputs something close to usable with a few modifications:

• nodeName need to be renamed to tagName
• text nodes do not support nodeName or tagName, and so the Node class will need modifying to guard against calling methods on node.tagName
• removeChild, used when collapsing whitespace, will need to be reimplemented
• node.getAttribute will also need to be reimplemented

I'm sure there'll be more breakages, but that's as far as I have got!

[fabiospampinato]

the current implementation requires it by default so that it can be readily used when turndown is called; after all, most developers will just have an HTML string they want to convert, rather than a DOM tree.

I think it may be better to require the expensive dependencies only when they are actually needed. For instance in Notable turndown is used only when/if the user imports an .enex file, there's no reason to pay for the time to require jsdom until it's actually needed.

But this is the minor problem, at least for my use case, I can lazily import it myself, but my bundled app is still considerably heavier than it needs to be because of jsdom.

It sort of comes down to fast, good, or cheap—choose two, where "fast" is performance, and "cheap" is refers to my time :D. So to summarise, my approach has been to accept HTML strings as input to make it easy-to-use, but with the option of using your own parser.

To be clear I'm not criticizing your design decisions, I would have probably made similar ones, I'm just saying that there's some big room for improvements and I'd be interested in seeing them.

I have been looking at using parse5, which I believe JSDOM uses behind the scenes.

I've taken a look at it and I see it has basically no dependencies, the whole thing may require less than 500kb, that's a huge improvement! If by using it turndown could also be kept mostly, if not completely, backwards compatible then that's even better.

@domchristie thank you for taking a deeper look at this!

[domchristie]

https://github.com/montagejs/minidom looks ideal for what we're after. With only a couple of changes, I have it running with 235/241 assertions passing. Most the failures look like they're to do with collapsing whitespace. This might be an issue with minidom's parse5 dependency (which is v1, whereas the latest parse5 is v5), but it could be that the collapse function is using attributes that are not supported in minidom.

[domchristie]

Down to 3 failing tests due to whitespace issues: https://github.com/domchristie/turndown/tree/minidom

[domchristie]

All tests are passing on https://github.com/domchristie/turndown/tree/minidom. I think I'll cherry-pick a few a commits from there into mater so that the library is mostly compatible with DOM Level 1 (+ textContentfrom level 3 and innerHTML and outerHTML), allowing minidom-generated DOM trees to be passed in.

I still like the features and familiarity that JSDOM provides, but I'm open to suggestions as to how we might reduce the performance cost, whilst maintaining the ease-of-use.

[domchristie]

I'm not familiar with turndown's internals, but I guess if turndowns receives a dom node rather than a string then maybe there's no need to require jsdom at all.

Thinking about this a bit more, I wonder if we create another package which only accepts DOM elements as input (e.g. turndown-core naming suggestions welcome!). Then Turndown could depend on that, and modify the turndown method to parse the input if necessary.

This has the benefits of maintaining the same API, whilst providing a JSDOM-free version for those happy to perform the parsing themselves. The downside is that it becomes a bit confusing for people to know where to raise issues, browse source code, and open pull requests.

[fabiospampinato]

@domchristie I'll play with the minidom branch and report back.

IMHO the best case scenario would be that the library maintains backwards compatibility but gets significantly less heavy. Are there any important features missing in minidom currently?

turndown-core

I would be happy to use that, but if possible I'd try to avoid releasing another entry point for turndown, the current situation is kind of complicated already.

Another option for Node.js builds could be this:

1. if turndown receives an object we don't load any third party DOM implementations
2. if it receives a string we look for a DOM implementation a. we check if we are in a browser context (maybe we are inside Electron's renderer process), something like global.window && global.window.document && global.window.document.toString () === '[object HTMLDocument]', and if so use the native implementation b. if not we try to load jsdom c. if that fails we try to load minidom d. if that fails too we throw an error, mentioning that no DOM implementation has been found and maybe the user forgot to install either jsdom or minidom. In this case maybe both jsdom and minidom should be defined as optional dependencies

This has some advantages:

1. It would be trivial to upgrade to the new version as for most people all they'll have to do would be to npm i jsdom
2. In case we are inside Electron's renderer process (which I admit might be a narrow use case perhaps) we don't even pay for minidom
3. No additional entry points are added and the library could be fully backwards compatible once jsdom gets installed

What do you think?

[domchristie]

Nice ideas.

IMHO the best case scenario would be that the library maintains backwards compatibility but gets significantly less heavy.

This would be ideal.

Are there any important features missing in minidom currently?

Unfortunately minidom only supports methods and properties from DOM level 1, which is rather restricting. For example, quering is limited to getElementsByTagname (no querySelector / querySelectorAll), and attributes have to be accessed via getAttribute rather than accessing the property directly. These are probably only minor inconveniences, but enough to be confusing when developers are so used to using them when working with the DOM.

I would be happy to use that, but if possible I'd try to avoid releasing another entry point for turndown, the current situation is kind of complicated already.

I agree.

In this case maybe both jsdom and minidom should be defined as optional dependencies … No additional entry points are added and the library could be fully backwards compatible once jsdom gets installed.

Interesting. I'd not heard of optional dependencies. I suppose the issue is that JSDOM's API may change (which it has done in the last few years). This could be guarded against but it might start to get messy.

Just to go reframe the issue. I think there are three areas where the performance could probably be improved:

1. npm i (downloading the excessive JSDOM dependencies)
2. require-ing JSDOM (particularly when it's not needed to parse anything)
3. Parsing HTML with JSDOM

A turndown-core package would effectively solve those, but I tend to agree that it's all a bit confusing. I'll have another think about how this might be better solved.

Thanks for your thought and input!

[Vaelatern]

For whatever it is worth, I use turndown exclusively from a webpage, so do not use nodejs. I only use npm to obtain the turndown source, for including in my webpage.

[fabiospampinato]

Interesting. I'd not heard of optional dependencies.

Actually those don't work as I thought they did, basically npm always tries to install them but if that fails the installation as a whole doesn't fail. What you want here are actually peer depedencies, which are dependencies npm doesn't try to install by itself but it gives a warning if they are unmet.

I suppose the issue is that JSDOM's API may change (which it has done in the last few years).

I think just locking down the peer dependency to a specific version, "jsdom": "1.2.3" or something perhaps a bit less restrictive, should be enough 🤔.

Also FYI I've just stumbled upon domino, a DOM level 4 implementation. The repo seems a bit on the heavy side (> 5mb) but most of that comes from the test directory.

Also maybe a new parser option could be added to turndown/turndown-core so that any custom DOM implementation could be used (as long as the core assumes at best a DOM level 1 implementation or something like that I suppose this should be doable):

new turndown ({
  parser: str => require ( 'my-dom-implementation' ).parse ( str )
}); 

Having still built-in support for Electron's renderer process and jsdom/minidom would still be nice though.

[fabiospampinato]

@domchristie I've just switched enex-dump to the minidom branch and the time to require turndown went from ~550ms to ~70ms, and the time for parsing a test .enex file went from ~100ms to ~50ms. Also because of the removal of jsdom and another heavy dependency of enex-dump the bundled version of Notable now weighs 11mb less.

That seems pretty good to me.

[domchristie]

@Vaelatern out of interest, which build are you importing, and how are you importing it? (I'm thinking in terms of https://github.com/domchristie/turndown/issues/265#issuecomment-447590252). If you're using a browser build, I suppose the main issue for you is the time is takes to run npm i?

[Vaelatern]

That, the difficulty of auditing my deps, and the extra size I never use but is baked in to my images.

I'm using turndown.browser.umd.js just because I picked one at random until it worked (I was also looking at the README while doing so), and I'm importing with a <script> tag in my webpage.

[domchristie]

Just to clarify, are you including <script src="PATH_TO/turndown.browser.umd.js"> directly in your page rather than including it (via require) as part of a compiled script? I'm wondering if there are any nice ways to import a turndown build other than the default.