GaurangTandon / ProKeys

free Chrome/Opera extension; makes you productive in online text-related work
Other
101 stars 32 forks source link

Performance improvement for content script when loading hundreds of snippets #296

Open GaurangTandon opened 5 years ago

GaurangTandon commented 5 years ago

Content script should query the background page every time user presses a hotkey (while also cancelling the event).

If the hotkey is valid, insert the snippet as before. If the hotkey is invalid, use chrome.debugger to dispatch the hotkey event using the key and code properties as usual. (link)

This will avoid the problem of every tab getting a list of hundreds of snippets and loading it in the tab.

So, the whole thing is almost done in branch performance-improvement. Now, we first need to upgrade the hotkey storage in #140 for v3.6.2 upgrade, and then push this upgrade out in v3.6.3. Reason being chrome debugger doesn't support keyCode but rather only key and code.

GaurangTandon commented 5 years ago

Using the chrome.debugger has these problems:

  1. Enter key doesn't work
  2. there is a small popup "prokeys is debugging the browser" everytime i type (if the hotkey is something simple like an a)
  3. requires new permission from user to update

The only solution I can think of to this is to disable Enter key and Tab key from being used, which is not at all ideal.

So, we'll use the selectionchange event. Note that it does not still work on FF. Have to post a question related to that.

GaurangTandon commented 5 years ago
  1. Use trie of reversed snippet names so we can directly match snippet name going from caretpos to caretpos - object_name_limit in reverse direction; also, use json representation for trie so that it can be persisted
  2. do not reconstruct trie on every deletion, insertion in options page; instead only modify that snippet name that was changed.

    Possible update operations affect presence of snippets in trie:

    a. created snippet b. edited snip name c. deleted snip d. clone existing snip e. bulk delete snips

    Track these operations and update trie based on them.

GaurangTandon commented 5 years ago

Selection change does not fire on: backspace, delete, ctrl-x. Hence, use throttle on both keydown and selectionchange event.

Also, no need of trie as object access in js is already O(1). And my Data.getUniqueSnip function is hence O(1)

GaurangTandon commented 5 years ago

Even at the latest commit (https://github.com/GaurangTandon/ProKeys/commit/b4ab33bba57ba40de6497c71d7e9bf3483ad52ce), this has the problem of taking up infinite memory in Chrome.

After binary searching by putting return statements in setPageDOM, I find that putting a return just after chrome.runtime.onMessage and just before attachNecessaryHandlers causes the issue as well. Hence, there is some message from background page that messes up my content script.

Further investigation reveals, putting a return before checkBlockedYourself send message fixes the issue. Finally figured out, trying to insert 6000 snippets into the context menu is crashing it.

GaurangTandon commented 5 years ago
This test code: ``` let starttime = Date.now(), TIME_LIMIT = 1000; // insert 100 ctx menu entries at once, stop when one second is over function insert100Ctx(multi = 0) { let finished = 0; for (let i = 0; i < 100; i++) { const str = (multi * 100 + i) + "|" + Math.random(); chrome.contextMenus.create({ contexts: ["editable"], id: str, title: str, }, () => finished++); } const checkInt = setInterval(() => { if (finished === 100) { clearInterval(checkInt); if (Date.now() - starttime < TIME_LIMIT) insert100Ctx(multi + 1); else {.log(multi, Date.now() - starttime); } } }, 1); } insert100Ctx(); ```

inserts as many snippets as it can into the context menu in one second. Turns out that figure is around 1200 snippets in 1.005 seconds approx. Also, upping the TIME_LIMIT to 10secs causes chrome to reach 2GB memory consumption in just around 2 secs.

I would believe therefore that a safe limit to number of context menu items is 500 items max. Any more than that and they should be clipped. How to clip them is something we need to decide. What do you think?

GaurangTandon commented 5 years ago

The currently demarcated safe limit is 800 snippets. It's really huge though.

We should note that given the infinite nesting we support, it is possible to have 10 main level folders, and 10 more folders within each. In this, we have 100 folders and even if each folder has five snippets, we have five hundred ctx menu entries. In this situation, it takes three simple snippets to navigate even to the 500-th snippet. So, it is not impossible that a user with a hundred snippets wouldn't use the context menu.