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

refactor: use stream #62

Merged
merged 1 commit into from
Oct 12, 2019
Merged

refactor: use stream #62

merged 1 commit into from
Oct 12, 2019

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Oct 11, 2019

This PR reverts #22 since the Database.save becomes 30x slower based on hexojs/hexo#3758

@SukkaW
Copy link
Member Author

SukkaW commented Oct 11, 2019

This is just a temporary solution. We should dig into this later.

@curbengh
Copy link
Contributor

The purpose of #22 was to prevent memory exhaustion. Using https://github.com/hexojs/site as test site (since it contains hundreds of files and used in hexojs/hexo#3758), I wonder what is the memory usage of current situation versus this PR? I will test in my workstation, but it's better if you can chime in as well.

@SukkaW
Copy link
Member Author

SukkaW commented Oct 11, 2019

@curbengh

Current master branch: about 300MB
This PR: about 214MB

I can see there is a lot of hard task on toJSON() (toJSON() is required by json-stream-stringify). Maybe this is why the process has been slowed down.

@curbengh
Copy link
Contributor

curbengh commented Oct 12, 2019

Current master branch: about 300MB
This PR: about 214MB

Lower memory huh.

I'll test this on my end by today.


I got higher memory usage.

Tested on a VM with a single CPU and 4GB RAM, using Node 8.16 (for worst case scenario). Two tests were conducted in separate folders.

  • Current master branch: about 643MB
  • This PR: about 818MB

Current master branch:

        User time (seconds): 102.23
        System time (seconds): 43.44
        Percent of CPU this job got: 97%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 2:28.88
        Maximum resident set size (kbytes): 642852
        Major (requiring I/O) page faults: 1
        Minor (reclaiming a frame) page faults: 246690
        Voluntary context switches: 8058418
        Involuntary context switches: 8064985
        File system inputs: 3272
        File system outputs: 255776

This PR:

        User time (seconds): 25.71
        System time (seconds): 2.11
        Percent of CPU this job got: 93%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:29.78
        Maximum resident set size (kbytes): 818296
        Major (requiring I/O) page faults: 3
        Minor (reclaiming a frame) page faults: 304279
        Voluntary context switches: 397640
        Involuntary context switches: 398217
        File system inputs: 135768
        File system outputs: 255744

Node 12.12, fresh npm install by removing package-lock beforehand.

Current:

        User time (seconds): 101.22
        System time (seconds): 43.25
        Percent of CPU this job got: 98%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 2:26.85
        Maximum resident set size (kbytes): 661176
        Major (requiring I/O) page faults: 1
        Minor (reclaiming a frame) page faults: 221941
        Voluntary context switches: 8035523
        Involuntary context switches: 8042855
        File system outputs: 255768

This PR:

        User time (seconds): 23.64
        System time (seconds): 4.72
        Percent of CPU this job got: 92%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:30.76
        Maximum resident set size (kbytes): 814040
        Major (requiring I/O) page faults: 1
        Minor (reclaiming a frame) page faults: 285037
        Voluntary context switches: 409505
        Involuntary context switches: 411386
        File system outputs: 255736

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.

Current approach sacrifices too much performance for memory. I concur with reverting, at least for now.

@curbengh curbengh merged commit 9bd2a2f into hexojs:master Oct 12, 2019
@curbengh curbengh mentioned this pull request Oct 12, 2019
@segayuu
Copy link
Contributor

segayuu commented Oct 12, 2019

I agree with the purpose of PR. Stream is a technique that prevents memory depletion at the expense of memory performance, but did not expect it to drop by a factor of 30.
However, #22 is the result of emphasizing maintainability while preventing memory exhaustion.
In order to improve memory performance while preventing memory exhaustion, we will need to implement it.

@stevenjoezhang stevenjoezhang linked an issue Mar 30, 2022 that may be closed by this pull request
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.

Why use stream for warehouse#save()?
3 participants