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

feat(cache): plain object cache #162

merged 7 commits into from Dec 19, 2019

Conversation

@SukkaW
Copy link
Member

SukkaW commented Dec 18, 2019

Context: #158

Cache with plain object.

cc @dailyrandomphoto @seaoak

@SukkaW SukkaW requested review from seaoak, curbengh and hexojs/core Dec 18, 2019
@coveralls

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

curbengh commented Dec 18, 2019

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

@SukkaW

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

curbengh commented Dec 18, 2019

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.

@SukkaW SukkaW requested a review from curbengh Dec 18, 2019
@SukkaW

This comment has been minimized.

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

This comment has been minimized.

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 Dec 19, 2019
@SukkaW SukkaW force-pushed the SukkaW:plain-object-cache branch from 7a5ded5 to afdc7f8 Dec 19, 2019
Copy link
Contributor

curbengh left a comment

lgtm

@SukkaW SukkaW merged commit 1a2041f into hexojs:master Dec 19, 2019
3 checks passed
3 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.2%) to 96.774%
Details
@seaoak

This comment has been minimized.

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

This comment has been minimized.

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 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.