Skip to content

refactor: use stream#62

Merged
curbengh merged 1 commit intohexojs:masterfrom
SukkaW:output_by_stream
Oct 12, 2019
Merged

refactor: use stream#62
curbengh merged 1 commit intohexojs:masterfrom
SukkaW:output_by_stream

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 SukkaW requested review from curbengh and segayuu October 11, 2019 07:53
@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

curbengh commented Oct 11, 2019

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 •
edited
Loading

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

curbengh
curbengh approved these changes Oct 12, 2019
View reviewed changes
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
Merged
@segayuu
Copy link
Contributor

segayuu commented Oct 12, 2019 •
edited
Loading

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.

@segayuu segayuu mentioned this pull request Oct 16, 2019
Merged
@stevenjoezhang stevenjoezhang mentioned this pull request Mar 30, 2022
Closed
@stevenjoezhang stevenjoezhang linked an issue Mar 30, 2022 that may be closed by this pull request
Why use stream for warehouse#save()? #20
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@segayuu segayuu segayuu approved these changes

@curbengh curbengh curbengh approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Why use stream for warehouse#save()?

3 participants

@SukkaW @curbengh @segayuu