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

Regression in CI benchmark #3833

Closed
curbengh opened this issue Nov 5, 2019 · 5 comments
Closed

Regression in CI benchmark #3833

curbengh opened this issue Nov 5, 2019 · 5 comments
Assignees

Comments

@curbengh
Copy link
Contributor

curbengh commented Nov 5, 2019

Usually cold processing can complete within 25 seconds, lately I noticed it has increased to more than a minute.

I haven't find the cause yet, I did notice the regression in #3808 #3812 #3813

Commit Time (GMT) Commit Cold Processing Time
(Node 8)
#3813
27 Oct 14:48 c8fee53 24s
29 Oct 12:26 330d88b 38s
#3808
28 Oct 06:39 28949da 23s
02 Nov 03:29 48d2c52 40s
#3812
27 Oct 13:32 3222513 23s
28 Oct 05:09 ab23935 35s

Reverting #3808 and #3813 in #3831, but no difference. Haven't try #3812 yet.

Commit Time (GMT) Commit PR Cold Processing Time
(Node 8)
Latest master
05 Nov 01:32 77f3e21 #3829 42s
Revert attempt
05 Nov 02:21 8a475a8 #3831 40s

I also tried dropping recent commits until 3448a6d, see revert-lodash tree. The processing time did come back to normal 26s (build log).


Looks like whatwg API is the cause #3812
I revert #3812 in #3834, processing time is 27s (build log).

In Node 13, it dropped from 81s to 27s.

@SukkaW
Copy link
Member

SukkaW commented Nov 5, 2019

Node.js 8 Node.js 10 Node.js 12 Node.js 13
Current master branch (#3829) 37s 33s 85s 83s
PR #3834 27s 27s 27s 26s
PR #3835 41s 33s 85s 82s

@curbengh

I have to say, the benchmark result is consistent. The problem must be at whatwg URL api.

@SukkaW
Copy link
Member

SukkaW commented Nov 6, 2019

@curbengh

I have setup a benchmark on runkit: https://runkit.com/sukkaw/5dc278520410ce00133cb012

It seems that whatwg url is definitely slower.

@curbengh
Copy link
Contributor Author

curbengh commented Nov 7, 2019

I just had an idea, since external_link filter incurs perf cost (regardless of legacy or whatwg), an alternative approach is to implement it under hexo-renderer-marked and hexo-renderer-markdown-it. This approach is more efficient because it is specific to links, compared to a filter that has to run through every content to look for links.

Having it under renderer means links in post/page can be applied, as long they are in []() markdown syntax. This means a limitation of renderer approach is that raw <a> (especially in theme layout) won't be applied. Another way to look at it is that the coverage is between external_link.field: post and external_link.field: site.

I still think external_link filter can be useful (especially the coverage offered by field: site), but disable it by default and enable it in renderer instead. Not now, perhaps in the next major version.

I intend to add the feature to hexo-renderer-marked in the coming days. It will be disabled by default.

@SukkaW
Copy link
Member

SukkaW commented Nov 7, 2019

@curbengh

I agree with that idea. We might add this to our Roadmap.

@curbengh
Copy link
Contributor Author

curbengh commented Nov 9, 2019

Discussion move to #3846.

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

Successfully merging a pull request may close this issue.

2 participants