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

A script in the extension "OctoLinker" is causing Firefox to slow down #557

Open
bfred-it opened this issue Jun 7, 2019 · 6 comments

Comments

3 participants
@bfred-it
Copy link
Contributor

commented Jun 7, 2019

I consistently get this message when visiting this large file:

A script in the extension  OctoLinker  is causing Firefox to slow down

https://github.com/Microsoft/TypeScript/blob/master/lib/lib.dom.d.ts

Admittedly this is more of an edge case, given the length of the file, but freezing the browser for a few seconds is never good.

Perhaps the operation can be done in the background and/or split up in multiple batches to avoid long blocking.

@stefanbuck

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Wow this file length is insane! I think it's best to not execute OctoLinker at all if a file goes beyond a certain line limit. Refactroing the extension to support such edgaces seems to be overkill and I would rather spend my time with new features.

Defining the limit is probaly the trickest task here. My gut feeling says that everyone beyone 5000 lines should be ignored.

How does this sound to you?

@bfred-it

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

That sounds alright, but it might be a good opportunity to find some bottlenecks, like these long garbage collection events

Screenshot 2019-06-07 at 19 31 59

It appears to be caused by this (which contains getAggregateText):

import findAndReplaceDOMText from 'findandreplacedomtext';

I'm sure that there's a more efficient way to replace dom elements, e.g. looping getTextNodes

Also from what I understand, the whole document is re-parsed for each regex

@silverwind

This comment has been minimized.

Copy link

commented Jun 7, 2019

I too regularily observer the browser tab hanging when opening moderately big package.json files. For example https://github.com/elastic/kibana/blob/master/package.json freezes my browser tab for almost 20 seconds, which I think is borderline unacceptable. A max line count won't help here as the density of links is just too high in such files.

If CPU-intensive work is to be done, maybe offload it to a web worker? Thought based on the comments above, the bottleneck sounds like inefficient parsing/replacing.

@bfred-it

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

freezes my browser tab for almost 20 seconds, which I think is borderline unacceptable.

20-seconds freezes aren't just borderline unacceptable

Wow, that file is only 440 lines but freezes the browser for longer than TypeScript's 18K lines file

@stefanbuck

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

So this issue isn't new for both of you and existed also in the previous version, right? Anyway, sorry for this bad experience.

@bfred-it I agree, there are more efficeient ways to replace dom elements, but maintaining this extension for many years, taught me that relying on classNames and dom elements is error-prone and time consuming to keep in sync. findandreplacedomtext was solving this issue for me pretty well.

OctoLinker does not apply all RegExps over and over again on the same document. First, all blobs on a docuemnt are paresed and stored in an unified format. Then based on the filepath and/or language information one or more plugins are applied to parse the blob. Usally it's only one apart from a few expections. A plugin can define one or more RegEx where each RegEx is applied on the blob, not the whole page. The JavaScript plugin contains three RegExp https://github.com/OctoLinker/OctoLinker/blob/master/packages/plugin-javascript/index.js#L108

Maybe this helps to track down the issue. I'll look into this, but feel free to add your ideas / explorations as well.

@silverwind

This comment has been minimized.

Copy link

commented Jun 8, 2019

Yes, this is not a new issue, it's been that way since I'm using the extension.

getAggregateText is here. It looks to extract a array of strings from the DOM. I think this is a operation that only needs to run once per page, maybe some memoization might help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.