test: reduce buffer size in buffer-creation test - #11177
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
|
@misterdjules Can this land now? |
Sorry, something went wrong.
misterdjules
left a comment
There was a problem hiding this comment.
The code changes look fine to me, but I find the commit message to be a bit confusing for two reasons:
-
this test still allocates "a huge chunk of memory"
-
the commit messages hints that it fixes the test on SmartOS, but we don't have the data to back this up
Can we rephrase the commit message as following:
"This test is allocating much more memory than necessary to actually reproduce the original problem. Lowering the amount of memory allocated increases performance at least in some cases and makes test/sequential/test-buffer-creation-regression.js less likely to time out on SmartOS."
Sorry, something went wrong.
|
@thefourtheye Sorry for the delay, I was on vacation until today. |
Sorry, something went wrong.
This test is allocating much more memory than necessary to actually reproduce the original problem. Lowering the amount of memory allocated increases performance at least in some cases and makes this test less likely to time out on SmartOS. Ref: nodejs#10166
|
@misterdjules No problem :-) Hope you had a good time. Your suggestion looks better than the current commit message so I updated the commit. PTAL. |
Sorry, something went wrong.
Sorry, something went wrong.
|
@thefourtheye Thank you for updating the commit message, it is very much appreciated! |
Sorry, something went wrong.
|
CI Run before landing: https://ci.nodejs.org/job/node-test-pull-request/7159/ |
Sorry, something went wrong.
This test is allocating much more memory than necessary to actually reproduce the original problem. Lowering the amount of memory allocated increases performance at least in some cases and makes this test less likely to time out on SmartOS. PR-URL: #11177 Ref: #10166 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Julien Gilli <jgilli@nodejs.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This test is allocating much more memory than necessary to actually reproduce the original problem. Lowering the amount of memory allocated increases performance at least in some cases and makes this test less likely to time out on SmartOS. PR-URL: nodejs#11177 Ref: nodejs#10166 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Julien Gilli <jgilli@nodejs.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This test is allocating much more memory than necessary to actually reproduce the original problem. Lowering the amount of memory allocated increases performance at least in some cases and makes this test less likely to time out on SmartOS. PR-URL: #11177 Ref: #10166 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Julien Gilli <jgilli@nodejs.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This test is allocating much more memory than necessary to actually reproduce the original problem. Lowering the amount of memory allocated increases performance at least in some cases and makes this test less likely to time out on SmartOS. PR-URL: #11177 Ref: #10166 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Julien Gilli <jgilli@nodejs.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This test is allocating much more memory than necessary to actually reproduce the original problem. Lowering the amount of memory allocated increases performance at least in some cases and makes this test less likely to time out on SmartOS. PR-URL: nodejs/node#11177 Ref: nodejs/node#10166 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Julien Gilli <jgilli@nodejs.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
As the test test/sequential/test-buffer-creation-regression.js fails
in SmartOS, it would be better not to allocate a huge chunk of memory.
Ref: #10166
Checklist
Affected core subsystem(s)
test