tools: fix prof polyfill readline#18641

killagu · 2018-02-08T13:04:11Z

node --prof foo.js may not print the full profile log file,
the last line will broken like tick, and not more blank line.
readlinewill stuck in infinite loop, add bytes < buf.length
will fix it.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

@bnoordhuis

killagu · 2018-02-08T13:09:50Z

node v9.4.0 and v9.5.0 may not print the full profile log file when use the ctrl-c close the process.

mmarchini · 2018-02-08T23:16:11Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

typo s/BORKEN_PART/BROKEN_PART/
Also, could we just use const BROKEN_PART = 'tick,'/ instead? "0x100840650" looks like a magic number here.

yes,it's better.

killagu · 2018-02-09T01:11:33Z

@mmarchini the magic number have been removed.

Fishrock123 · 2018-02-10T17:08:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13082/

Fishrock123 · 2018-02-10T17:08:40Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

I'm not sure whether the linter (make lint) will complain or not, but this is difficult to read. Please use braces here.

XadillaX · 2018-02-11T03:02:31Z

lib/internal/v8_prof_polyfill.js

emit a warning while broken?

`node --prof foo.js` may not print the full profile log file, the last line will broken like `tick,` and not more blank line. `readline`will stuck in infinite loop, add `bytes === 0` will fix it.

XadillaX

LGTM if CI is OK.

XadillaX · 2018-02-11T03:53:14Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/13101/

XadillaX · 2018-02-11T04:06:30Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/13102/

XadillaX · 2018-02-11T09:22:02Z

test/common/index.js

 exports.restoreStdout=restoreWritable.bind(null,'stdout');
 exports.restoreStderr=restoreWritable.bind(null,'stderr');
+exports.isSymbolAvailable=exports.isWindows||
+exports.isSunOS||


nit: align with exports above.

@XadillaX as you wish.

cjihrig · 2018-02-11T20:03:26Z

test/common/README.md


 Platform check for Windows 32-bit on Windows 64-bit.

+### isSymbolAvailable


Can we use a more descriptive name than this please?

my bad,it should be isCPPSymbolsNotMapped. @cjihrig

cjihrig · 2018-02-11T20:04:13Z

test/common/README.md

+### isSymbolAvailable
+*[&lt;Boolean>]
+
+Platform check for if symbol available.


Similarly, can you provide a more in depth description.

killagu · 2018-02-12T01:16:40Z

@cjihrig the property name,and readme have been changed to more descriptive.

XadillaX · 2018-02-12T02:14:48Z

test/common/README.md

 *[&lt;Boolean>]

-Platform check for if symbol available.
+Platform check for c++ symbols are mapped or not.


c++ => C++

mmarchini

CI: https://ci.nodejs.org/job/node-test-pull-request/13149/

BridgeAR · 2018-02-16T21:28:34Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

+tmpdir.refresh();
+
+if(!common.enoughTestCpu)
+common.skip('test is CPU-intensive');


Nit: would you be so kind and use a capital letter as first character? :-)

In the node test,the first character in skip message is always lower case.

BridgeAR · 2018-02-16T21:29:11Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

+common.skip('test is CPU-intensive');
+
+if(common.isCPPSymbolsNotMapped){
+common.skip('C++ symbols are not mapped for this os.');


Nit: Operating system should be upper cased as in OS.

Thank you for the advice.It should be upper cased.

BridgeAR · 2018-02-16T21:29:54Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

+
+//This test will produce a broken profile log.
+//ensure prof-polyfill not stuck in infinite loop
+//and success process


Nit: please use a space at the beginning of each line in a comment.

Thank you for the advice.It should have a space at the beginning of each line.

BridgeAR · 2018-02-16T21:46:07Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

+ this.ts = Date.now();
+ setImmediate(function(){new f()});
+ };
+ f();`;


Nit: we have the unwritten rule not to use multi line template strings.

Thank you for the advice.I will split it to two lines.

BridgeAR · 2018-02-16T21:46:43Z

test/tick-processor/test-tick-processor-polyfill-brokenfile.js

+'--prof-process',LOG_FILE
+]);
+assert(WARN_REG_EXP.test(child.stderr.toString()));
+assert.strictEqual(child.status,0,'broken file should success too');


I personally would like to stick to the default message.

The default message will be removed.

BridgeAR · 2018-02-16T21:50:13Z

lib/internal/v8_prof_polyfill.js

 return'';
 }
+if(bytes===0){
+process.emitWarning('profile file is broken',{


Suggestion: you might just add the filename of the broken file? And please use a capital letter as first character.

Thank you for the advice.Add the filename is better.

fix warn message fix comment style

killagu · 2018-02-17T00:49:39Z

@BridgeAR Thank you for the advice.I have update the code.Please review it.

BridgeAR · 2018-02-17T01:19:32Z

CI https://ci.nodejs.org/job/node-test-pull-request/13231/

`node --prof foo.js` may not print the full profile log file, leaving the last line broken (for example `tick,`. When that happens, `readline` will be stuck in an infinite loop. This patch fixes it. Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code on tick-processor tests. PR-URL: #18641 Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

mmarchini · 2018-02-17T12:09:25Z

Landed in 38f04d4 😄

MylesBorins · 2018-02-21T15:11:13Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

killagu · 2018-02-21T15:33:38Z

@MylesBorins It should be backported to v9.x-staging,and I will raise a backport PR.

`node --prof foo.js` may not print the full profile log file, leaving the last line broken (for example `tick,`. When that happens, `readline` will be stuck in an infinite loop. This patch fixes it. Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code on tick-processor tests. PR-URL: nodejs#18641 Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

killagu · 2018-02-21T16:26:22Z

@MylesBorins I have raised a PR to backport it.

BTW I have no permission for run the CI. If you can help me trigger the CI, you are so kind.

`node --prof foo.js` may not print the full profile log file, leaving the last line broken (for example `tick,`. When that happens, `readline` will be stuck in an infinite loop. This patch fixes it. Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code on tick-processor tests. PR-URL: nodejs#18641 Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

`node --prof foo.js` may not print the full profile log file, leaving the last line broken (for example `tick,`. When that happens, `readline` will be stuck in an infinite loop. This patch fixes it. Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code on tick-processor tests. Backport-PR-URL: #18901 PR-URL: #18641 Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

`node --prof foo.js` may not print the full profile log file, leaving the last line broken (for example `tick,`. When that happens, `readline` will be stuck in an infinite loop. This patch fixes it. Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code on tick-processor tests. PR-URL: nodejs#18641 Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

`node --prof foo.js` may not print the full profile log file, leaving the last line broken (for example `tick,`. When that happens, `readline` will be stuck in an infinite loop. This patch fixes it. Also introduced `common.isCPPSymbolsNotMapped` to avoid duplicated code on tick-processor tests. Backport-PR-URL: #18901 PR-URL: #18641 Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Feb 8, 2018

mmarchini reviewed Feb 8, 2018
View reviewed changes

killagu force-pushed the fix-prof-polyfill branch from 7b4e27c to bfaee12Compare February 9, 2018 01:10

Fishrock123 reviewed Feb 10, 2018
View reviewed changes

killagu force-pushed the fix-prof-polyfill branch 2 times, most recently from 479f168 to fd9baf5Compare February 11, 2018 02:33

XadillaX reviewed Feb 11, 2018
View reviewed changes

lib/internal/v8_prof_polyfill.js Outdated
Copy link
Contributor
XadillaXFeb 11, 2018
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emit a warning while broken?

killagu force-pushed the fix-prof-polyfill branch from 9b54b62 to 41f92a4Compare February 11, 2018 03:47

tools: fix prof polyfill readline
33f4a51
`node --prof foo.js` may not print the full profile log file, the last line will broken like `tick,` and not more blank line. `readline`will stuck in infinite loop, add `bytes === 0` will fix it.

killagu force-pushed the fix-prof-polyfill branch from 41f92a4 to 33f4a51Compare February 11, 2018 03:50

XadillaX approved these changes Feb 11, 2018
View reviewed changes

test: add common.isSymbolAvailable
45730d5

XadillaX reviewed Feb 11, 2018
View reviewed changes

fix: fix code style
2610183

cjihrig reviewed Feb 11, 2018
View reviewed changes

mv common.isSymbolAvailable -> common.isCPPSymbolsNotMapped
05b0391

XadillaX reviewed Feb 12, 2018
View reviewed changes

fix c++ -> C++
3684bc0

mmarchini approved these changes Feb 13, 2018
View reviewed changes

BridgeAR reviewed Feb 16, 2018
View reviewed changes

fix code style
a43951e
fix warn message fix comment style

BridgeAR approved these changes Feb 17, 2018
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018

mmarchini closed this Feb 17, 2018

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

killagu mentioned this pull request Feb 21, 2018
[v9.x backport] tools, test: fix prof polyfill readline #18900
Closed
4 tasks

killagu mentioned this pull request Feb 21, 2018
[v9.x backport] tools, test: fix prof polyfill readline #18901
Closed
2 tasks

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Feb 26, 2018

addaleax mentioned this pull request Feb 27, 2018
v9.7.0 proposal #19040
Closed

MylesBorins added the land-on-v8.x label Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018
v8.12.0 proposal #21593
Merged


		Platform check for Windows 32-bit on Windows 64-bit.

		### isSymbolAvailable

Uh oh!

tools: fix prof polyfill readline#18641

tools: fix prof polyfill readline #18641

Uh oh!

Conversation

killagu commented Feb 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

killagu commented Feb 8, 2018

Uh oh!

mmarchiniFeb 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

killagu commented Feb 9, 2018

Uh oh!

Fishrock123 commented Feb 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XadillaX left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XadillaX commented Feb 11, 2018

Uh oh!

XadillaX commented Feb 11, 2018

Uh oh!

XadillaXFeb 11, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

killaguFeb 12, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

killagu commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmarchini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

killagu commented Feb 17, 2018

killagu commented Feb 8, 2018•
edited
Loading

mmarchiniFeb 8, 2018•
edited
Loading

XadillaX left a comment •
edited
Loading

XadillaXFeb 11, 2018•
edited
Loading

killaguFeb 12, 2018•
edited
Loading