OnContentChanged Is Heavy On Large Documents, But Debouncing Drops Changes

[vkoves]

I've been doing a lot of performance investigation into my company's use of Quill, and have found that onContentChanged is calling getSemanticHtml (see quill-editor.component.ts:418, here in textChangeHandler), which on a large document (20k words) can take tens of milliseconds, causing significant lag when typing.

I then found issue #1226, but unfortunately the debounced onContentChanged doesn't debounce deltas in the way I'd expect (you get the change delta since the last emission) but just drops changes, only emitting the last change, making properly tracking each change and parsing impossible. Here's some documentation of these two - in the second, I typed "Two friends", but only see changes for "o" and "s", where I paused.

Flamegraph Showing onContentChanged getHtml Calls	Debounced onContentChanged Logs Showing Partial Deltas

I wanted to see if:

1. Do you have any idea why it looks like getSemanticHtml is getting called twice per keypress? The flamegraph doesn't make clear where the second call is coming from, but it's inside ngx-quill
2. Would debouncing just HTML and text changes make sense? Quill itself doesn't seem to struggle with large documents, so I think just the HTML conversion is the big problem.

I'm happy to take a crack at opening a PR for any fixes, just want to make sure I have alignment and any guidance or tips!

Update: Just to show that QuillJS doesn't struggle with large docs, here's a screenshot showing the interaction times with 20,000 words:

QuillJS Screenshot

[KillerCodeMonkey]

I know that the content is getting twice this can be optimized very easy.
Maybe i have the time and mood to do it first.

in the quill event handlers in the quill-editor-component i grab the content in all available formats, because i want unified output signals providing everything the user may need.
But i call the "valueGetter" again for the model change.

But priority for know is to refactor everything to switch to zoneless change detection.
#2007

I guess the the quill playground / website editor is just using and giving you internally the "obejct" Delta representation.. no big html document handling at all.

And now we are coming to the root cause... it is not recommended to work with html at all.
I considered removing the html support, because it is unsafe and is not working well with quill 2. But then a lot of projects will have problems.
Maybe i will put this under a feature flag, if i am at that point. So you are actively decide to take the performance and other disadvantages on your own risk.

[KillerCodeMonkey]

@vkoves try 29.0.1 please

[KillerCodeMonkey]

@vkoves in 29.1.0 you have a new input onlyFormatEventData so the handlers are getting called only with format related content. So if your are not using html -> no getSemanticHTML calls should be done

[vkoves]

@KillerCodeMonkey - thank you, I'll take a peek on Monday when I'm back in office!

I think the HTML question is a tricky one - I've leaned on HTML because we have a sanitization service to prevent security concerns, and the HTML content makes it easier to migrate to another editor later on if needed, plus we can render the HTML without Quill (improving performance and reducing JS bundle size for pages just rendering content). I'm very hesitant to store Quill deltas in a database, since it's basically a one-off format, compared to fairly standard HTML. We may well be the outliers here, but I wanted to provide some context for why folks might be choosing to use HTML even if it's not recommended.

[vkoves]

@KillerCodeMonkey - looking at this now, I think your solution is helpful, but won't help in our particular case - we are using the HTML format, but I want to debounce the HTML conversion only, while still tracking delta changes. We do a bunch of custom handling using the delta on each change (hence why we can't debounce that), but the HTML processing itself we could debounce. Perhaps the option could be renamed to richEventData or something, defaulting to true to pass text and HTML?

I think to your other point though, even dropping the duplicate getHTML calls would cut the latency in half, but I think it's still too heavy to do on every keypress.

[KillerCodeMonkey]

A possibility would be to just avoid using ngx-quill at all or instead of using the outputs grab the editor instance and subscribe to the native quill events and do what ever you want with it.

I could imagine to extend my introduced input by the value "none" so you can again just get the deltas.

[vkoves]

@KillerCodeMonkey - great suggestions! I suppose I could also just change the format we are using and do the conversions myself, which would achieve the same outcome - lots of options.

[KillerCodeMonkey]

Yes. But i do not see, what i can do here that you can debounce it and stay with the functionality as it is.

I want to stay at close to native quill behavior as possible. To be able to maintain it at all

[vkoves]

@KillerCodeMonkey - I think I'll figure something out, thank you for your help!

Just FYI though, I don't think your onlyFormatEventData change quite works - there's still a call to getSemanticHtml due to valueGetter - it improves performance by reducing it from two calls to one, but it seems a bit odd that you're still calling getSemanticHtml and just not returning the HTML in the event (see offending code in quill-editor.compoennt.ts:180).

Flamegraph	Related Code causing getSemanticHtml

It might be worth writing a functional test that confirms that after a keystroke getSemanticHtml is not called with your new setting, and one validating that it's only called once per keypress in the format: html mode. We're implementing similar safety checks in our editor component to ensure that developers don't add in new getSemanticHtml calls, as well as some performance tests that run functions a dozen times with a large (20,000 word) document and confirm it runs within a few milliseconds. It's really easy to add these performance regressions, and it can be a royal pain to root them out.

[KillerCodeMonkey]

In your case there will be a call, cause ypur format is html.

It is more a perf improvement for all other cases ;-).

I have a test case that if the setting is true only the format the editor uses is passed to the change handler.

In your case they will be one call for now.
But as described in a comment above that i may extend that input with "none" as value, so you just get deltas without any getsemantichtml calls, when format is html.

[vkoves]

@KillerCodeMonkey - I think you misunderstand, I was interested in testing your performance improvement and switched the format to object as you noted, tested with the latest version, and still saw the getSemanticHTML call. I then traced it to the valueGetter, which will always call getSemanticHtml - it's the first line, with no conditional.

Proof that I'm using it properly:

The code I mentioned above is quill-editor.component.ts:180

[vkoves]

Basically I'm saying that valueGetter shouldn't call getSemanticHtml, because it's then not even using that value if the format is not HTML. If it is, the text change handler could reuse that with valueGetterValue.

[KillerCodeMonkey]

Ah good catch. I did not check the getter

In my brain i thought the getter just gets the content for the format.

Will check and change it tomorrow.

If you need it now, it should be possible to override the value getter

[vkoves]

@KillerCodeMonkey - all good, thank you! I'm not in a rush with this, just wanted to point out the performance issue since I spotted it.

I think we've optimized it to a point I'm fairly happy with, but one less getSemanticHtml call will definitely speed things up 😁