Convert self.body to a Buffer if it's a String to reduce mem usage #2051

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@hassy

hassy commented Feb 2, 2016

No description provided.

@hassy

This comment has been minimized.

Show comment
Hide comment
@hassy

hassy Feb 2, 2016

A performance regression was introduced in: 478e0c2#diff-ccc0734f65dd7a299409ff07d35be095L436 Strings have a larger overhead than Buffers, which can cause the process to run out of memory faster when POSTing large payloads.

This might be an edge case, but it's something I ran into in Artillery which uses Request for HTTP, and it is a regression in performance after 2.58.0.

Test case here:

I can create an isolated test case for the test suite if this ok to be merged. 👌

hassy commented Feb 2, 2016

A performance regression was introduced in: 478e0c2#diff-ccc0734f65dd7a299409ff07d35be095L436 Strings have a larger overhead than Buffers, which can cause the process to run out of memory faster when POSTing large payloads.

This might be an edge case, but it's something I ran into in Artillery which uses Request for HTTP, and it is a regression in performance after 2.58.0.

Test case here:

I can create an isolated test case for the test suite if this ok to be merged. 👌

@FredKSchott

This comment has been minimized.

Show comment
Hide comment
@FredKSchott

FredKSchott Feb 22, 2017

Contributor

@hassy still interested in merging? Happy to work with you to review if so.

Contributor

FredKSchott commented Feb 22, 2017

@hassy still interested in merging? Happy to work with you to review if so.

@FredKSchott

This comment has been minimized.

Show comment
Hide comment
@FredKSchott

FredKSchott Feb 22, 2017

Contributor

This PR has been closed as "abandoned" as a part of an automated cleanup. If this is a mistake and you are still interested in merging this PR, please do the following:

  1. Rebase your changes onto the master branch (even if there are no merge conflicts, we want to make sure your change is under the most recent tests).
  2. Push your updated branch to Github to update this PR.
  3. Make sure this PR's Travis tests are still passing. (Run npm test locally if tests are still failing).
  4. Comment here to let us know that you've updated the PR and are still interested in getting it merged. Somebody from the project will respond and work with you to review.

Thank you for your understanding. If you have any questions, please check out #2556 and feel free to comment / ask anything there.

Contributor

FredKSchott commented Feb 22, 2017

This PR has been closed as "abandoned" as a part of an automated cleanup. If this is a mistake and you are still interested in merging this PR, please do the following:

  1. Rebase your changes onto the master branch (even if there are no merge conflicts, we want to make sure your change is under the most recent tests).
  2. Push your updated branch to Github to update this PR.
  3. Make sure this PR's Travis tests are still passing. (Run npm test locally if tests are still failing).
  4. Comment here to let us know that you've updated the PR and are still interested in getting it merged. Somebody from the project will respond and work with you to review.

Thank you for your understanding. If you have any questions, please check out #2556 and feel free to comment / ask anything there.

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