Skip to content
This repository has been archived by the owner on May 20, 2020. It is now read-only.

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

Open
GaurangTandon opened this issue Jun 28, 2019 · 6 comments
Assignees
Labels
approved enhancement that will be made enhancement snippet-insertion
Milestone

Comments

@GaurangTandon
Copy link
Owner

GaurangTandon commented Jun 28, 2019

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 GaurangTandon added this to the 3.6.3 milestone Jun 28, 2019
@GaurangTandon GaurangTandon self-assigned this Jun 28, 2019
@GaurangTandon
Copy link
Owner Author

GaurangTandon commented Jun 28, 2019

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
Copy link
Owner Author

GaurangTandon commented Jun 29, 2019

  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
Copy link
Owner Author

GaurangTandon commented Jun 29, 2019

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
Copy link
Owner Author

GaurangTandon commented Jun 29, 2019

Even at the latest commit (b4ab33b), 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
Copy link
Owner Author

GaurangTandon commented Jun 29, 2019

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 added a commit that referenced this issue Jul 1, 2019
@GaurangTandon
Copy link
Owner Author

GaurangTandon commented Jul 1, 2019

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.

@GaurangTandon GaurangTandon modified the milestones: 3.6.3, 3.7.0 Jul 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved enhancement that will be made enhancement snippet-insertion
Projects
None yet
Development

No branches or pull requests

1 participant