Loading a compare view of a minified diff is very slow

[kumar303]

trafficstars

Describe the problem and steps to reproduce it:

Here's how I can reproduce it in my local environment:

• sign in to dev
• go to https://code.addons-dev.allizom.org/en-US/compare/502955/versions/1541798...1541798/?path=popup%2Fpopup.js

What happened?

It takes 20 seconds to load popup.js and Firefox shows the slow bar asking if I want to kill the web page

What did you expect to happen?

It should either load faster or say the diff is too big or something like that

Anything else we should know?

This was a problem with the old tool and, surprise, it's a problem with the new one!

The API request is http://localhost:3000/api/v4dev/reviewers/addon/502955/versions/1541798/compare_to/1541798/?file=popup/popup.json&lang=en-US

It responded in 844ms so it's not an API problem.

It's a JS / layout problem. Performance profile: https://perfht.ml/2TDrfVL

┆Issue is synchronized with this Jira Task

[willdurand]

Oh, I see. That looks like a problem related to syntax highlighting and not with the diff. There are not a lot of changes in the API response. If you disable options.highlight in DiffView, it renders quit fast.

Given that it renders quite fast in the CodeView too, I believe the issue is related to the syntax HL in the DiffView component, likely because it tries to "apply" syntax HL on each token.

[kumar303]

Yep, syntax highlighting was going to be my guess. It wasn't obvious to me from looking at the perf profile. Hmm. I need to learn how to analyze those better.

[kumar303]

The quickest fix here is probably to disable syntax highlighting when we detect minification (long lines or something) then offer a button to put highlighting back, maybe.

[willdurand]

We could defer syntax HL in a web worker to avoid blocking the main thread: https://github.com/otakustay/react-diff-view/blob/6aa5399c52392e19f7f8fbe4af17b374b4339862/src/hocs/withTokenizeWorker.js

[ioanarusiczki]

I've noticed this issue reproducible for https://code.addons-dev.allizom.org/en-US/compare/390151/versions/1688198...1688302/ https://code.addons-dev.allizom.org/en-US/compare/495710/versions/1541610...1688333/ https://code.addons.allizom.org/en-US/compare/1002795/versions/2474840...2474841/

[wagnerand]

Is #779 a dupe of this?

[kumar303]

Yes, maybe. There are a couple others that may be dupes but I was planning to check the STR and URLs for each to make sure we cover all cases.

[kumar303]

We have a workaround in https://github.com/mozilla/addons-code-manager/issues/942 which should speed this up but, really, we can do syntax highlighting faster once https://github.com/mozilla/addons-code-manager/issues/928 lands so I'm going to keep this open as a way to test it.

[bobsilverberg]

@wagnerand This is actually fixed now, because we introduced code that disables highlighting for large files. It is being kept open in order to test/verify #928 when it lands.

1. We should perhaps remove the mvp label from this, and bump it down the priority list as it's not currently reproducible.
2. I wonder if we should bump #928 up the priority list. If we can fix it it might allow us to re-introduce syntax highlighting for large files, but perhaps that's not a particularly high priority item? We've already discussed the fact that there's not much point in highlighting minified files as they're virtually unreadable anyway, which is another reason that #928 might not be that high a priority. I just wanted to raise it to get your opinion.

[wagnerand]

@bobsilverberg Sounds good, thanks! No need to bump #928 at the moment, but we might revisit that later.