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

Memoize in isVisible using a LRU cache #121

Open
biancadanforth opened this issue Jul 29, 2019 · 1 comment
Open

Memoize in isVisible using a LRU cache #121

biancadanforth opened this issue Jul 29, 2019 · 1 comment

Comments

@biancadanforth
Copy link
Collaborator

Related to: #91

#118 made isVisible more than 50% faster by changing the implementation to check for clickability of an element rather than a bunch of styles for it and its ancestors.

It does not currently memoize the results, however, due to some challenges including running into an infinite recursion with the note method.

Due to limitations of the note method[1], we would likely be better served to take a more Map-like approach. However, one concern about the Map approach is that, as a global variable, it can grow in an unbounded fashion and consume a lot of memory (for example, the sample Amazon page I used for profiling in PR #118 had upwards of 6000 elements). We should be able to limit its RAM use if we implement a LRU (least recently used) cache and limit the maximum number of elements stored in the cache.

[1]: Per Erik, the note method assumes one rule writes the note while another rule reads it. Also, isVisible doesn't know what type of fnode we're dealing with, and notes are based on type. Changing these assumptions for the note method would require some significant work.

@erikrose
Copy link
Contributor

erikrose commented Jul 29, 2019 via email

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

No branches or pull requests

2 participants