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] feat(cache): bring up lfu #3901

Closed
wants to merge 2 commits into from

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Dec 2, 2019

What does it do?

This PR is a part of proposal #3897, bring up LFU cache for fragment_cache().

How to test

git clone -b fragment_cache_lru https://github.com/sukkaw/hexo.git
cd hexo
npm install
npm test

Screenshots

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 97.136% when pulling dd14ff2 on SukkaW:fragment_cache_lru into e423773 on hexojs:master.

@coveralls
Copy link

coveralls commented Dec 2, 2019

Coverage Status

Coverage increased (+0.0008%) to 97.136% when pulling c7c34cd on SukkaW:fragment_cache_lru into e423773 on hexojs:master.

@SukkaW SukkaW changed the title feat(fragment_cache): bring up lru feat(fragment_cache): bring up lfu Dec 2, 2019
@SukkaW SukkaW requested review from curbengh, yoshinorin, segayuu and a team and removed request for curbengh and yoshinorin December 2, 2019 14:14
module.exports = ctx => {
let cache = {};
const cache = new LRU(50);
Copy link
Member Author

@SukkaW SukkaW Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have run some test by using Object.keys(cache).length to know the size of cache for some popular themes:

Theme Current cache size
hexo-theme-light 1
hexo-theme-tranquilpeak 3
hexo-theme-landscape 4
hexo-theme-cake 17
hexo-theme-next 18
hexo-theme-suka 18
hexo-theme-icarus 98

Those data are from one-language site. Multi-language site could have larger size of cache.

Maybe 50 is still to large. We could use 15 or 20 (I prefer 20).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is best to be exposed to the outside for configuration, If some theme has larger cache length, it can adjust it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of users have no idea about the cache size of their themes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of users have no idea about the cache size of their themes.

But as a theme developer. If he knew clearly that he needed 50+, how to achieve it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiangtj In fact he/she doesn't. It is better to cache as less as possible because larger size of cache means higher memory usage. It is not a wise idea to cache everything in the fragment_cache. That's how LFU works, by only caching the most usefuls.

@SukkaW SukkaW changed the title feat(fragment_cache): bring up lfu feat(cache): bring up lfu Dec 3, 2019
@SukkaW SukkaW changed the title feat(cache): bring up lfu [WIP] feat(cache): bring up lfu & enhance cache for helpers Dec 3, 2019
@SukkaW SukkaW changed the title [WIP] feat(cache): bring up lfu & enhance cache for helpers [WIP] feat(cache): bring up lfu Dec 3, 2019
@SukkaW SukkaW closed this Dec 6, 2019
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

Successfully merging this pull request may close these issues.

3 participants