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

WIP: Optimize shadow hinting #3269

Closed

Conversation

heiwiper
Copy link
Contributor

@heiwiper heiwiper commented Dec 2, 2023

Description

This draft PR should address the performance issue of hint-mode which significantly slows down hint overlays rendering when performing the follow-hint command in a page with thousands of hintable elements.

Fixes #3254

Discussion

The change that has been introduced so far is to separate the flet declared function in the macros nyxt/ps:rqsa and nyxt/ps:rqs-nyxt-id which is suspected to cause some delay, considering that a lambda function has to be declared during every call to these two macros.

I have also optimized the two recursive functions which are now macros. Instead of walking through all DOM elements to check for shadow root elements, a query is now performed using the existing custom class nyxt-shadow-root. Note that there's still need to confirm that this change does not introduce any regression bugs, so I would like to ask for more guidance about how to write a test for hint-mode.

Apparently, the performance hasn't improved much compared to using the macros qsa and qs-nyxt-id. The next part that I will try to tackle is the call to nyxt/ps:rqs-nyxt-id in the function nyxt/mode/hint:hint-elements, I will try see whether it would be possible to refactor that part of the code.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

  • Git hygiene:
    • I have pulled from master before submitting this PR
    • There are no merge conflicts.
  • I've added the new dependencies as:
    • ASDF dependencies,
    • Git submodules,
      cd /path/to/nyxt/checkout
      git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser
    • and Guix dependencies.
  • My code follows the style guidelines for Common Lisp code. See:
  • I have performed a self-review of my own code.
  • My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
  • Documentation:
    • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
    • I have updated the existing documentation to match my changes.
    • I have commented my code in hard-to-understand areas.
    • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
      • Changelog update should be a separate commit.
    • I have added a migration.lisp entry for all compatibility-breaking changes.
    • (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainters to do so.
  • Compilation and tests:
    • My changes generate no new warnings.
    • I have added tests that prove my fix is effective or that my feature works. (If possible.)
    • I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

Avoids iterating through all DOM nodes to walk through shadow DOM
elements. Instead, we make use of the nyxt class 'nyxt-shadow-dom'.

Note that the use of 'do' loop macro in 'rqs-nyxt-id' makes sure we get that
parenscript generates the desired code in javascript.
Avoid using 'flet' in 'rqsa' and 'rqs-nyxt-id'. And move the functions into a
separate parenscript macros.
@aadcg
Copy link
Member

aadcg commented Dec 4, 2023

Note that there's still need to confirm that this change does not introduce any regression bugs, so I would like to ask for more guidance about how to write a test for hint-mode.

@heiwiper let's keep that task outside the scope of this PR. Feel free to open an issue about it, and I'll share how I'd do it.

To validate that there are no performance penalties, take a URL with many links and compare the performance before commit b43ec71 and at the tip of the working branch. URL https://en.wikipedia.org/wiki/Europe is a good candidate, since it hits almost 5k hints.

I'll mark this PR as a draft.

@aadcg aadcg marked this pull request as draft December 4, 2023 17:53
@aadcg aadcg mentioned this pull request Dec 4, 2023
@aadcg
Copy link
Member

aadcg commented Dec 19, 2023

@heiwiper I've briefly meditated on the subject and we may be looking at the issue in a fundamental wrong way. The performance penalty may be coming purely from the inadequacy of the JS algorithm (and PS is thus irrelevant).

I believe you have followed the implementation from saka-key (which, btw, is a draft and isn't used anywhere). I think that a better source of inspiration would be the one from LinkHints.

Let me know if you'd like to work on it or if I should take over it. Thanks.

For future reference, a test URL for this feature - https://developer.servicenow.com/dev.do.

@heiwiper
Copy link
Contributor Author

Hello André, I haven't had much time to work on this PR lately. I'm switching over to a new job and I'm using some of my free time to adapt to the new tech stack at work. I believe I won't be able to get back to contributing until around the end of the next month. So even though I would like to work on this issue, I would understand if someone else takes over due to the high priority.

Sorry for not communicating my unavailability earlier.

@aadcg
Copy link
Member

aadcg commented Dec 21, 2023

@heiwiper no worries! I hope you're doing well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

hint-mode performance
2 participants