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

feat(cache): plain object cache #162

Merged
merged 7 commits into from
Dec 19, 2019
Merged

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Dec 18, 2019

Context: #158

Cache with plain object.

cc @dailyrandomphoto @seaoak

@SukkaW SukkaW requested review from seaoak, curbengh and a team December 18, 2019 07:54
@coveralls
Copy link

coveralls commented Dec 18, 2019

Coverage Status

Coverage increased (+0.2%) to 96.774% when pulling afdc7f8 on SukkaW:plain-object-cache into f80d6c6 on hexojs:master.

@SukkaW
Copy link
Member Author

SukkaW commented Dec 18, 2019

Benchmark result:

Node.js Version Cold Process Time Memory
8 Current master branch of Hexo 28.3s 587MB
8 Hexo with sukkaw/hexo-util#plain-object-cache 26.3s 589MB
10 Current master branch of Hexo 19.3s 596MB
10 Hexo with sukkaw/hexo-util#plain-object-cache 21.7s 600MB
12 Current master branch of Hexo 19.0s 582MB
12 Hexo with sukkaw/hexo-util#plain-object-cache 16.8s 580MB
13 Current master branch of Hexo 24.9s 587MB
13 Hexo with sukkaw/hexo-util#plain-object-cache 22.1s 590MB

@SukkaW SukkaW mentioned this pull request Dec 18, 2019
@curbengh
Copy link
Contributor

curbengh commented Dec 18, 2019

I notice repetitive codes, perhaps can refactor into a cache.js (a new utility)?

@SukkaW
Copy link
Member Author

SukkaW commented Dec 18, 2019

@curbengh I see no necessary.

I have bring up something like this:

// cache.js

'use strict';

module.exports = class Cache {
  constructor() {
    this.cache = {};
  }

  set(id, value) {
    this.cache[id] = value;
  }

  has(id) {
    return this.cache[id] != null;
  }

  get(id) {
    return this.cache[id];
  }

  flush() {
    this.cache = {};
  }
};

But the usage will end in something like:

const Cache = require('./cache');
const cache = new Cache();

if (cache.has(cacheId)) return cache.get(cacheId);
...
cache.set(cacheId, value);

Nothing changed...

@curbengh
Copy link
Contributor

const Cache = require('./cache');
const cache = new Cache();

if (cache.has(cacheId)) return cache.get(cacheId);
...
cache.set(cacheId, value);

while it doesn't reduce the line, it does look more readable.

curbengh
curbengh previously approved these changes Dec 18, 2019
@SukkaW SukkaW requested a review from curbengh December 18, 2019 12:36
@SukkaW
Copy link
Member Author

SukkaW commented Dec 18, 2019

@curbengh I have bring up cache.js with set(), get(), has(), del(), flush() and apply() methods. cache.apply() has similar usage of fragment_cache. In fact we could utilize fragment_cache helper using cache.apply.

@SukkaW
Copy link
Member Author

SukkaW commented Dec 18, 2019

Performance with Cache():

Node.js Version Cold Process Time Memory
8 Current master branch of Hexo 25.2s 591MB
8 Hexo with sukkaw/hexo-util#plain-object-cache 20.0s 603MB
10 Current master branch of Hexo 18.7s 593MB
10 Hexo with sukkaw/hexo-util#plain-object-cache 22.9s 585MB
12 Current master branch of Hexo 18.4s 576MB
12 Hexo with sukkaw/hexo-util#plain-object-cache 17.2s 584MB
13 Current master branch of Hexo 18.8s 642MB
13 Hexo with sukkaw/hexo-util#plain-object-cache 16.9s 624MB

lib/cache.js Outdated Show resolved Hide resolved
@SukkaW SukkaW requested a review from seaoak December 19, 2019 04:53
@SukkaW SukkaW force-pushed the plain-object-cache branch from 7a5ded5 to afdc7f8 Compare December 19, 2019 05:07
Copy link
Contributor

@curbengh curbengh left a comment

Choose a reason for hiding this comment

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

lgtm

@SukkaW SukkaW merged commit 1a2041f into hexojs:master Dec 19, 2019
@seaoak
Copy link
Member

seaoak commented Dec 19, 2019

I tasted this patch in a large blog (dummy 3000 posts).

The result shows this patch is 13% faster than master.
(no regression!)


Details:

Measure elapsed time of hexo generate --debug.
For each condition, measure 10 times and calculate the average.
Before each measurement, do hexo clean and rm debug.log.
Set CPU affinity 0x00010000 and priority HIGH for hexo command (using START command).

Result

Elapsed time:

Branch Average Spread (worst - best)
master 77.34sec 1.67sec
SukkaW 67.15sec (-13.18%) 2.44sec

Source code:

Environment:

  • OS: Windows 10 64bit
  • CPU: Ryzen Threadripper 2950X (16core/32thread, 3.5GHz)
  • MEM: 32GB (8GB x4) DDR4 (3600MHz)
  • SSD: Samsung EVO970 (1TB)
  • Node v12.14.0

@seaoak
Copy link
Member

seaoak commented Dec 19, 2019

Measure elapsed time of hexo generate --debug.

Memory usage is about 2.3GB as the worst case in both conditions.
(measured with tasklist command)

@SukkaW SukkaW mentioned this pull request Dec 21, 2019
1 task
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.

4 participants