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

Streaming Warehouse#save() #64

Merged
merged 2 commits into from
Oct 28, 2019
Merged

Conversation

segayuu
Copy link
Contributor

@segayuu segayuu commented Oct 16, 2019

This PR is a continuation of #62 .
One reason for the poor memory performance of #22 was that there was one Readable and one Writable, which were connected by a pipe().
Since only one Stream is a huge object, we thought the possibility that the increase in Readable led to memory pressure.
The purpose of this PR is to verify whether memory consumption can be reduced while implementing interruption / resumption of processing by Bluebird.coroutine().
I don't know the details of the method of performance testing, but at least the testing of the Warehouse alone did not change the processing speed significantly.

@curbengh
Copy link
Contributor

curbengh commented Oct 16, 2019

Current:

	User time (seconds): 26.53
	System time (seconds): 1.87
	Percent of CPU this job got: 96%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:29.53
	Maximum resident set size (kbytes): 813140

This PR:

	User time (seconds): 27.05
	System time (seconds): 1.80
	Percent of CPU this job got: 96%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:30.01
	Maximum resident set size (kbytes): 825764

@curbengh
Copy link
Contributor

I'll do another round of benchmark.

@SukkaW
Copy link
Member

SukkaW commented Oct 18, 2019

I have run a benchmark:

Hexo 4.0 NodeJS 8 NodeJS 10 NodeJS 12
Load Files 6.9s / 2.87s 6.06s / 2.59s 6.06s / 2.55s
Genrate Files 11s / 10s 8.99s / 8.7s 9.18s / 8,95s
Save Database 0.7s / 0.6s 0.6s / 0.8s 0.51s / 0.45s
This PR NodeJS 8 NodeJS 10 NodeJS 12
Load Files 6.8s / 2.8s 6.09s / 2.57s 6.09s / 2.6s
Genrate Files 11s / 10s 8.99s / 8.65s 9.24s / 8.97s
Save Database 0.6s / 0.6s 0.6s / 0.7s 0.45s / 0.48s

(Cold Processing / Hot Processing)

I think it is ok to go.

@curbengh
Copy link
Contributor

@SukkaW
what about memory consumption (which is the main purpose of this PR)?

@SukkaW
Copy link
Member

SukkaW commented Oct 20, 2019

@curbengh

Hexo 4.0:

NodeJS 8 NodeJS 10 NodeJS 12
532.5MB / 504MB 438.7MB / 494MB 464MB / 483.7MB

This PR:

NodeJS 8 NodeJS 10 NodeJS 12
535MB / 511MB 402MB / 486MB 462MB / 473.5MB

(Cold Processing / Hot Processing)

@curbengh
Copy link
Contributor

Ran benchmark on Travis, using hexo.io as test website.

Current master branch:

Node 8 10 12
909MB 976MB 908MB

This PR:

Node 8 10 12
921MB 952MB 913MB

Even including SukkaW's result, I feel the differences in performance and memory consumption are within margin of error.

Anyhow, this PR introduces backpressure detection, (which in my current understanding) is that it could minimize out-of-memory issue. So, this PR might benefit most in low RAM environment. I'll do another test in 2GB/1.5GB or maybe even 1GB environment.

@segayuu
Copy link
Contributor Author

segayuu commented Oct 20, 2019

Actually, it is not interrupted / resumed by backpressure, but it is interrupted at each model._export() and resumed as soon as its output is complete.
The implementation of fs.WriteStream calls fs.write() and fs.writev() directly. If stream.write() is simply called, IO processing is generated accordingly.
Writable.cork() / uncork() is used and fs.writev() is used to reduce IO processing (node ​​v8 does not have fs.writev() itself, but there is something equivalent to internal API) .
If fs.writev() can always be called, or even without fs.WriteStream, there is a possibility that it can be exported efficiently.

@curbengh
Copy link
Contributor

I just tested with 1GB RAM, and the differences are not significant.

Writable.cork() / uncork() is used and fs.writev() is used to reduce IO processing

Perhaps this is why my benchmark didn't show any difference, because it didn't test this.

Overall, current evidence doesn't show this PR introduce any regression. I don't mind this PR, even though there's still no convincing evidence that it can significantly improve perf or reduce memory.

It's a limitation of the benchmark I used (since it doesn't target Node app), perhaps someone can help benchmark IO processing, but again, I'm not suggesting to wait for another round of benchmark.

@segayuu
Copy link
Contributor Author

segayuu commented Oct 28, 2019

Thanks to @curbengh and @SukkaW review.
Since both reviews have settled and there is no objection, we will merge.

@segayuu segayuu merged commit c467f21 into hexojs:master Oct 28, 2019
@segayuu segayuu deleted the warehouse#save-streaming branch October 28, 2019 03:51
@SukkaW SukkaW mentioned this pull request Jan 4, 2020
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.

None yet

3 participants