perf: memorize encoded summary article by Yilun-Sun · Pull Request #243 · honkit/honkit

Yilun-Sun · 2021-12-22T06:28:11Z

memorize encoded summary article

azu · 2021-12-24T02:25:23Z

packages/honkit/src/json/encodeSummaryArticle.ts

+const articleMapByLevel = new Map();
+
 function encodeSummaryArticle(article: SummaryArticle, recursive?: boolean) {
+    const articleLevel = article.getLevel();


Is articleLevelunique in all articles?
Map allow you to use object as key.
articleCacheMap.set(article, encodedArticle); is better.
(lru cache is better for this usecase. to avoid memory leak)

It is better that separate memorize function like memorizeEncodeSummaryArticle.
The memorizeEncodeSummaryArticle(cached) will wrap the encodeSummaryArticle(non cache).
Or, encodeSummaryArticle(cached) and encodeSummaryArticleWithoutCache(non cache).
(It means that we can avoid to rename refererences)

If you can, please add some tests.

encodeSummaryWithoutCache (non cached) should return an object.

encodeSummary (cached) should return an object.

encodeSummary(cached) should return different object when pass difference SummaryArticle.

encodeSummary(cached) should return same object when pass same SummaryArticle.

assert.strictEqual(encodeSummary(article), encodeSummary(article)). it is just===.

encodeSummary and encodeSummaryWithoutCache should return equal object.

assert.deepStrictEqual(encodeSummary(article), encodeSummaryWithoutCache(article))

refs: SummaryArticle.create will be used for this test.

https://github.com/honkit/honkit/blob/4a439e5aff23b0722b93ab30186f2a30872a12e4/packages/honkit/src/models/__tests__/summaryArticle.ts

Got it! Thanks for the suggestions, I am going to work on that!

Yilun-Sun · 2021-12-31T07:04:45Z

@azu I am not sure where should I use the encodeSummaryArticleWithCache for. So I only use in encodeSummaryArticle.ts.
And for the articleLevel. It is unique in all articles.
See here: https://github.com/honkit/honkit/blob/master/packages/honkit/src/models/summaryArticle.ts#:~:text=const%20articles%20%3D%20(def,%7D)%3B
So maybe we could use article level for the key of the map?

azu · 2022-01-02T06:41:22Z

And for the articleLevel. It is unique in all articles.

Ah, Ok.

{parentLevel}.{childIndex}

1.1
2.1
- 2.1.1
- 2.1.2
  - 2.1.2.1
  - 2.1.2.2

Yor're right, articleLevel seems be unique.

azu

I am not sure where should I use the encodeSummaryArticleWithCache for. So I only use in encodeSummaryArticle.ts.

"Go to references" on encodeSummaryArticle.
You can see the references of encodeSummaryArticle function.

or search encodeSummaryArticle (but it is not complete because default export can be renamed)
https://github.com/honkit/honkit/search?q=encodeSummaryArticle

azu · 2022-01-02T06:46:25Z

packages/honkit/src/json/encodeSummaryArticle.ts

        articles = article
            .getArticles()
-            .map((article) => encodeSummaryArticle(article))
+            .map((article) => encodeSummaryArticleWithCache(article))


encodeSummaryArticle should not depend on encodeSummaryArticleWithCache.
This dependency prevent correct testing.
(We can not test encodeSummaryArticle(without cacehe) correctly).

Instead of it, encodeSummaryArticleWithCache depended on encodeSummaryArticle.

azu · 2022-01-02T06:53:31Z

packages/honkit/src/json/encodeSummaryArticleWithCache.ts

+    let articles = undefined;
+    if (recursive !== false) {
+        articles = article
+            .getArticles()
+            .map((article) => encodeSummaryArticleWithCache(article))
+            .toJS();
+    }
+
+    const encodedArticle = {
+        title: article.getTitle(),
+        level: article.getLevel(),
+        depth: article.getDepth(),
+        anchor: article.getAnchor(),
+        url: article.getUrl(),
+        path: article.getPath(),
+        ref: article.getRef(),
+        articles: articles,
+    };


Remove this lines and just use encodeSummaryArticle.

const encodedArticle = encodeSummaryArticle(article, recursive); articleCacheMap.set(article, encodedArticle);

As as result, we can create cached version of encodeSummaryArticle.
encoding article one time.
(encodeSummaryArticleWithCache depended on encodeSummaryArticle.)

@azu Sorry I don't understand. If we replace all current encodeSummaryArticle with encodeSummaryArticleWithCache and call encodeSummaryArticle inside encodeSummaryArticleWithCache. How could we cache articles? The first call of encodeSummaryArticleWithCache will go direct into encodeSummaryArticle and parse all articles recursively and only store the root one. So one article will still be encoded may times.

Or maybe I got lost, could you please tell me more about your thoughts? Thanks!

Ah, You're correct.

I think It is a bit strange depdency, but Let's move on.
https://github.com/honkit/honkit/pull/243/files#r777172419

azu · 2022-01-02T07:10:32Z

My commented approach: cache root article includes articles(children)
Current PR approach: cache each article wihtout root article

Another approach: always cache all article. (It always use encodeSummaryArticleWithCache and remove encodeSummaryArticle function.)

azu

LGTM

azu · 2022-01-17T13:12:47Z

packages/honkit/src/json/encodeSummaryArticleWithCache.ts

+    let articles = undefined;
+    if (recursive !== false) {
+        articles = article
+            .getArticles()
+            .map((article) => encodeSummaryArticleWithCache(article))
+            .toJS();
+    }
+
+    const encodedArticle = {
+        title: article.getTitle(),
+        level: article.getLevel(),
+        depth: article.getDepth(),
+        anchor: article.getAnchor(),
+        url: article.getUrl(),
+        path: article.getPath(),
+        ref: article.getRef(),
+        articles: articles,
+    };


Ah, You're correct.

I think It is a bit strange depdency, but Let's move on.
https://github.com/honkit/honkit/pull/243/files#r777172419

azu · 2022-01-17T13:13:06Z

Thanks for PR!

perf: memorize encoded summary article

e134cef

azu reviewed Dec 24, 2021

View reviewed changes

azu mentioned this pull request Dec 29, 2021

Build performance #245

Open

perf: improve encode summary article with memorized hash map

a6c9402

fix: change calling place of encode summary article with cache

a63b18c

azu reviewed Jan 2, 2022

View reviewed changes

azu approved these changes Jan 17, 2022

View reviewed changes

azu merged commit 6b2292d into honkit:master Jan 17, 2022

This was referenced Jan 17, 2022

refactor: use LRU_MAP #248

Merged

v3.7.0 #249

Merged

Uh oh!

Conversation

Yilun-Sun commented Dec 22, 2021

Uh oh!

azu Dec 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yilun-Sun Dec 24, 2021

Choose a reason for hiding this comment

Uh oh!

Yilun-Sun commented Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azu commented Jan 2, 2022

Uh oh!

azu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azu Jan 2, 2022

Choose a reason for hiding this comment

Uh oh!

azu Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yilun-Sun Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

azu Jan 17, 2022

Choose a reason for hiding this comment

Uh oh!

azu commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azu left a comment

Choose a reason for hiding this comment

Uh oh!

azu Jan 17, 2022

Choose a reason for hiding this comment

Uh oh!

azu commented Jan 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

azu Dec 24, 2021 •

edited

Loading

Yilun-Sun commented Dec 31, 2021 •

edited

Loading

azu left a comment •

edited

Loading

azu Jan 2, 2022 •

edited

Loading

azu commented Jan 2, 2022 •

edited

Loading