Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1884493003:
Dump C++ symbols and merge into v8 log. (Closed)
DescriptionDump C++ symbols and merge into v8 log.
Currently we already have tools to extract C++ symbols of d8, and output the
statistics result. This patch creates two scripts, one is to use exsisting tools
to extract C++ symbols and dump to output, the other collects all symbols and
merges them into v8 log. The format of C++ symbols in v8 log is:
cpp,start_address,size,symbol_name
BUG=v8:4906
LOG=n
Committed: https://crrev.com/aff441937d6bb1e2869772414ac5f0b0e0480143
Cr-Commit-Position: refs/heads/master@{#35841}
Patch Set 1 #Patch Set 2 : Update similarity #
Total comments: 1
Patch Set 3 : Support other platform #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 10
Patch Set 8 : #
Total comments: 14
Patch Set 9 : #
Total comments: 26
Patch Set 10 : #
Total comments: 2
Patch Set 11 : #
Messages
Total messages: 34 (9 generated)
Description was changed from ========== Dump C++ symbols in tick processor. tickprocessor.js has the ability to get C++ symbols for d8, in this patch we make it dump all C++ symbols to stdout and add a script to collect them and merge into log file. BUG=v8:4906 LOG=n ========== to ========== Dump C++ symbols in tick processor. tickprocessor.js has the ability to get C++ symbols for d8, in this patch we make it dump all C++ symbols to stdout and add a script to collect them and merge into log file. BUG=v8:4906 LOG=n ==========
lpy@chromium.org changed reviewers: + fmeawad@chromium.org
https://codereview.chromium.org/1884493003/diff/20001/tools/tickprocessor.js File tools/tickprocessor.js (right): https://codereview.chromium.org/1884493003/diff/20001/tools/tickprocessor.js#... tools/tickprocessor.js:794: '--dump-cpp': ['dumpCppEntries', true, I think it should be dump-cpp-symbols and the description to be clear as well, the flag name should convey the functionality. https://codereview.chromium.org/1884493003/diff/60001/tools/tickprocessor.js File tools/tickprocessor.js (right): https://codereview.chromium.org/1884493003/diff/60001/tools/tickprocessor.js#... tools/tickprocessor.js:794: '--dump-cpp': ['dumpCppEntries', true, All the other args manipulate the current output as is, but this one changes the output completely. I wonder if you should have a dedicated js file that uses codemap.js instead?
On 2016/04/14 11:42:50, fmeawad wrote: > https://codereview.chromium.org/1884493003/diff/20001/tools/tickprocessor.js > File tools/tickprocessor.js (right): > > https://codereview.chromium.org/1884493003/diff/20001/tools/tickprocessor.js#... > tools/tickprocessor.js:794: '--dump-cpp': ['dumpCppEntries', true, > I think it should be dump-cpp-symbols and the description to be clear as well, > the flag name should convey the functionality. > > https://codereview.chromium.org/1884493003/diff/60001/tools/tickprocessor.js > File tools/tickprocessor.js (right): > > https://codereview.chromium.org/1884493003/diff/60001/tools/tickprocessor.js#... > tools/tickprocessor.js:794: '--dump-cpp': ['dumpCppEntries', true, > All the other args manipulate the current output as is, but this one changes the > output completely. I wonder if you should have a dedicated js file that uses > codemap.js instead? Thanks, I think having a separate js file like you said is better. I updated the CL.
https://codereview.chromium.org/1884493003/diff/120001/tools/dump-cpp.py File tools/dump-cpp.py (right): https://codereview.chromium.org/1884493003/diff/120001/tools/dump-cpp.py#newc... tools/dump-cpp.py:43: JS_FILES = ['splaytree.js', 'codemap.js', 'csvparser.js', 'consarray.js', 'profile.js', 'logreader.js', 'tickprocessor.js', 'SourceMap.js', 'dump-cpp.js'] loose the "-" in the file name, all other file names have not dash. (in dump-cpp.js) https://codereview.chromium.org/1884493003/diff/120001/tools/dump-cpp.py#newc... tools/dump-cpp.py:88: if on_windows and out: Have you tried it on a windows machine? https://codereview.chromium.org/1884493003/diff/120001/tools/dump-cpp.py#newc... tools/dump-cpp.py:91: iswritten = False if out else True is_written https://codereview.chromium.org/1884493003/diff/120001/tools/tickprocessor.js File tools/tickprocessor.js (right): https://codereview.chromium.org/1884493003/diff/120001/tools/tickprocessor.js... tools/tickprocessor.js:785: '--dump-cpp-symbols': ['dumpCppSymbols', true, I think you should remove that flag. https://codereview.chromium.org/1884493003/diff/120001/tools/tickprocessor.js... tools/tickprocessor.js:828: dumpCppSymbols: false, ditto
I updated the CL. Thanks. https://codereview.chromium.org/1884493003/diff/120001/tools/dump-cpp.py File tools/dump-cpp.py (right): https://codereview.chromium.org/1884493003/diff/120001/tools/dump-cpp.py#newc... tools/dump-cpp.py:43: JS_FILES = ['splaytree.js', 'codemap.js', 'csvparser.js', 'consarray.js', 'profile.js', 'logreader.js', 'tickprocessor.js', 'SourceMap.js', 'dump-cpp.js'] On 2016/04/19 12:52:25, fmeawad wrote: > loose the "-" in the file name, all other file names have not dash. (in > dump-cpp.js) Done. https://codereview.chromium.org/1884493003/diff/120001/tools/dump-cpp.py#newc... tools/dump-cpp.py:88: if on_windows and out: On 2016/04/19 12:52:25, fmeawad wrote: > Have you tried it on a windows machine? No I haven't tried it on windows. https://codereview.chromium.org/1884493003/diff/120001/tools/dump-cpp.py#newc... tools/dump-cpp.py:91: iswritten = False if out else True On 2016/04/19 12:52:25, fmeawad wrote: > is_written Done. https://codereview.chromium.org/1884493003/diff/120001/tools/tickprocessor.js File tools/tickprocessor.js (right): https://codereview.chromium.org/1884493003/diff/120001/tools/tickprocessor.js... tools/tickprocessor.js:785: '--dump-cpp-symbols': ['dumpCppSymbols', true, On 2016/04/19 12:52:25, fmeawad wrote: > I think you should remove that flag. Done. https://codereview.chromium.org/1884493003/diff/120001/tools/tickprocessor.js... tools/tickprocessor.js:828: dumpCppSymbols: false, On 2016/04/19 12:52:25, fmeawad wrote: > ditto Done.
Also I will update the description later, since we no longer modify tickprocessor.
Description was changed from
==========
Dump C++ symbols in tick processor.
tickprocessor.js has the ability to get C++ symbols for d8, in this patch we
make it dump all C++ symbols to stdout and add a script to collect them and
merge into log file.
BUG=v8:4906
LOG=n
==========
to
==========
Dump C++ symbols and merge into v8 log.
Currently we already have tools to extract C++ symbols of d8, and output the
statistics result. This patch creates two scripts, one is to use exsisting tools
to extract C++ symbols and dump to output, the other collects all symbols and
merges them into v8 log. The format of C++ symbols in v8 log is:
cpp,start_address,size,symbol_name
BUG=v8:4906
LOG=n
==========
Some comments, also is there a chance we can add a test case? https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py File tools/dump-cpp.py (right): https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:30: # This script executes the tickprocessor, collects all dumped C++ symbols, It no longer executes the "tickprocessor" https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:31: # and merges them into v8.log. back into ... https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:44: 'profile.js', 'logreader.js', 'tickprocessor.js', 'SourceMap.js', Do we still need the tickprocessor.js? can we avoid that? https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:72: print 'No d8 binary path found in {}.'.format(log_file) I think that would give you the path to v8.log? https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:78: args = [d8_exec] + JS_FILES + ['--'] + args I do not see a different between the if and else clauses? is it something that looks the same? https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:86: if returncode != 0: No need to store the return code, just use it.
I think it's not necessary to add the test since we are running a binary from outside. https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py File tools/dump-cpp.py (right): https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:30: # This script executes the tickprocessor, collects all dumped C++ symbols, On 2016/04/20 22:54:22, fmeawad wrote: > It no longer executes the "tickprocessor" Done. https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:31: # and merges them into v8.log. On 2016/04/20 22:54:22, fmeawad wrote: > back into ... Done. https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:44: 'profile.js', 'logreader.js', 'tickprocessor.js', 'SourceMap.js', On 2016/04/20 22:54:22, fmeawad wrote: > Do we still need the tickprocessor.js? can we avoid that? Yes, we need to import it. https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:72: print 'No d8 binary path found in {}.'.format(log_file) On 2016/04/20 22:54:21, fmeawad wrote: > I think that would give you the path to v8.log? I am not sure if it's guaranteed that we can have the d8 path from v8 log. https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:78: args = [d8_exec] + JS_FILES + ['--'] + args On 2016/04/20 22:54:21, fmeawad wrote: > I do not see a different between the if and else clauses? is it something that > looks the same? Done. https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:86: if returncode != 0: On 2016/04/20 22:54:22, fmeawad wrote: > No need to store the return code, just use it. Done.
We need to at least be able to test the CppProcessor https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py File tools/dump-cpp.py (right): https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:44: 'profile.js', 'logreader.js', 'tickprocessor.js', 'SourceMap.js', On 2016/04/20 23:10:37, lpy wrote: > On 2016/04/20 22:54:22, fmeawad wrote: > > Do we still need the tickprocessor.js? can we avoid that? > > Yes, we need to import it. We should make the functionality in tickprocessor that is needed as part of a util or something, we should not depend on it. https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... tools/dump-cpp.py:72: print 'No d8 binary path found in {}.'.format(log_file) On 2016/04/20 23:10:37, lpy wrote: > On 2016/04/20 22:54:21, fmeawad wrote: > > I think that would give you the path to v8.log? > > I am not sure if it's guaranteed that we can have the d8 path from v8 log. Then the error message is misleading
On 2016/04/20 23:15:11, fmeawad wrote: > We need to at least be able to test the CppProcessor In addition to print symbols to stdout, CppProcessor just makes function calls to code map, cpp entries provider, etc. > https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py > File tools/dump-cpp.py (right): > > https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... > tools/dump-cpp.py:44: 'profile.js', 'logreader.js', 'tickprocessor.js', > 'SourceMap.js', > On 2016/04/20 23:10:37, lpy wrote: > > On 2016/04/20 22:54:22, fmeawad wrote: > > > Do we still need the tickprocessor.js? can we avoid that? > > > > Yes, we need to import it. > > We should make the functionality in tickprocessor that is needed as part of a > util or something, we should not depend on it. We can separate cppentriesprovider as an independent file, and also ArgumentsProcessor. Should we do it in another CL? > https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... > tools/dump-cpp.py:72: print 'No d8 binary path found in {}.'.format(log_file) > On 2016/04/20 23:10:37, lpy wrote: > > On 2016/04/20 22:54:21, fmeawad wrote: > > > I think that would give you the path to v8.log? > > > > I am not sure if it's guaranteed that we can have the d8 path from v8 log. > > Then the error message is misleading Why? The error message is saying the script can't find d8 binary path in |log_file_name|.
On 2016/04/20 23:30:43, lpy wrote: > On 2016/04/20 23:15:11, fmeawad wrote: > > We need to at least be able to test the CppProcessor > > In addition to print symbols to stdout, CppProcessor just makes function calls > to code map, cpp entries provider, etc. > The point of the test is to have a cpp symbolization framework test that can be used in any further cpp symbolization related work. > > > https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py > > File tools/dump-cpp.py (right): > > > > > https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... > > tools/dump-cpp.py:44: 'profile.js', 'logreader.js', 'tickprocessor.js', > > 'SourceMap.js', > > On 2016/04/20 23:10:37, lpy wrote: > > > On 2016/04/20 22:54:22, fmeawad wrote: > > > > Do we still need the tickprocessor.js? can we avoid that? > > > > > > Yes, we need to import it. > > > > We should make the functionality in tickprocessor that is needed as part of a > > util or something, we should not depend on it. > > We can separate cppentriesprovider as an independent file, and also > ArgumentsProcessor. > > Should we do it in another CL? > I think you should have it in this CL > > > > https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... > > tools/dump-cpp.py:72: print 'No d8 binary path found in {}.'.format(log_file) > > On 2016/04/20 23:10:37, lpy wrote: > > > On 2016/04/20 22:54:21, fmeawad wrote: > > > > I think that would give you the path to v8.log? > > > > > > I am not sure if it's guaranteed that we can have the d8 path from v8 log. > > > > Then the error message is misleading > > Why? The error message is saying the script can't find d8 binary path in > |log_file_name|. it is OK, my point was the the v8.log path is not indicative of the d8 binary path.
On 2016/04/21 00:54:11, fmeawad wrote: > On 2016/04/20 23:30:43, lpy wrote: > > On 2016/04/20 23:15:11, fmeawad wrote: > > > We need to at least be able to test the CppProcessor > > > > In addition to print symbols to stdout, CppProcessor just makes function calls > > to code map, cpp entries provider, etc. > > > The point of the test is to have a cpp symbolization framework test that can be > used in any further cpp symbolization related work. > > > > > https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py > > > File tools/dump-cpp.py (right): > > > > > > > > > https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... > > > tools/dump-cpp.py:44: 'profile.js', 'logreader.js', 'tickprocessor.js', > > > 'SourceMap.js', > > > On 2016/04/20 23:10:37, lpy wrote: > > > > On 2016/04/20 22:54:22, fmeawad wrote: > > > > > Do we still need the tickprocessor.js? can we avoid that? > > > > > > > > Yes, we need to import it. > > > > > > We should make the functionality in tickprocessor that is needed as part of > a > > > util or something, we should not depend on it. > > > > We can separate cppentriesprovider as an independent file, and also > > ArgumentsProcessor. > > > > Should we do it in another CL? > > > I think you should have it in this CL > > > > > > > > https://codereview.chromium.org/1884493003/diff/140001/tools/dump-cpp.py#newc... > > > tools/dump-cpp.py:72: print 'No d8 binary path found in > {}.'.format(log_file) > > > On 2016/04/20 23:10:37, lpy wrote: > > > > On 2016/04/20 22:54:21, fmeawad wrote: > > > > > I think that would give you the path to v8.log? > > > > > > > > I am not sure if it's guaranteed that we can have the d8 path from v8 log. > > > > > > Then the error message is misleading > > > > Why? The error message is saying the script can't find d8 binary path in > > |log_file_name|. > it is OK, my point was the the v8.log path is not indicative of the d8 binary > path. It seems that the current tools do not have tests, so we can skip that for now. Also we can split off the tickprocessor functionality in a separate CL. LGTM from my side.
lpy@chromium.org changed reviewers: + machenbach@chromium.org, yangguo@chromium.org
Thanks. +yang@ and machenbach@, PTAL.
On 2016/04/22 17:41:13, lpy wrote: > Thanks. > > +yang@ and machenbach@, PTAL. gentle ping for review ;)
machenbach@chromium.org changed reviewers: + jarin@chromium.org
I have a few python readability comments and suggestions. JS rubberstamp. +jarin for the general plans with our profiling tools. Also, I'd really like to get some meaningful subdirs into the tools dir but lets do this in another CL. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py File tools/dump-cpp.py (right): https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:2: # nit: Use short copyright header for new files. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:39: def isFileExecutable(fPath): nit: I prefer consistent underscore method names, like is_file_executable https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:47: on_windows = True if platform.system() == 'Windows' else False on_windows = platform.system() == 'Windows' https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:48: for i in range(0, len(JS_FILES)): Readability: JS_FILES = [os.path.join(tools_path, f) for f in JS_FILES] https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:54: for i in range(1, len(sys.argv)): Readability: for arg in sys.argv[1:]: # s/sys.argv[i]/arg https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:70: d8_exec = d8_line.groups()[0] d8_exec = d8_line.groups()[0] equivalent to d8_exec = d8_line.group(1) https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:73: exit(-1) Rather sys.exit(-1) in scripts I guess. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:77: with open(log_file) as f: I'm not familiar with this pattern. Will it keep the logfile open until all data is processed by sp.communicate()? What if the process is very slow? Wouldn't the with clause close the file before it's exhausted? https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:89: is_written = False if out else True Readability: is_written = not out or if you prefer: is_written = not bool(out) https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:90: with open(log_file, 'w+') as f: How about just w as you only write to the file? https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:93: f.write(out) So you append all symbols right after the first tick? https://codereview.chromium.org/1884493003/diff/160001/tools/dumpcpp.js File tools/dumpcpp.js (right): https://codereview.chromium.org/1884493003/diff/160001/tools/dumpcpp.js#newcode1 tools/dumpcpp.js:1: // Copyright 2016 the V8 project authors. All rights reserved. nit: Use short copyright header for new files.
looking good. https://codereview.chromium.org/1884493003/diff/160001/tools/dumpcpp.js File tools/dumpcpp.js (right): https://codereview.chromium.org/1884493003/diff/160001/tools/dumpcpp.js#newco... tools/dumpcpp.js:41: // Pull dev tools source maps into our name space. weird whitespace between "maps" and "into"
Thanks. I updated the CL. PTAL. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py File tools/dump-cpp.py (right): https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:2: # On 2016/04/26 12:16:10, Michael Achenbach wrote: > nit: Use short copyright header for new files. Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:39: def isFileExecutable(fPath): On 2016/04/26 12:16:10, Michael Achenbach wrote: > nit: I prefer consistent underscore method names, like is_file_executable Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:47: on_windows = True if platform.system() == 'Windows' else False On 2016/04/26 12:16:10, Michael Achenbach wrote: > on_windows = platform.system() == 'Windows' Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:48: for i in range(0, len(JS_FILES)): On 2016/04/26 12:16:10, Michael Achenbach wrote: > Readability: > JS_FILES = [os.path.join(tools_path, f) for f in JS_FILES] Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:54: for i in range(1, len(sys.argv)): On 2016/04/26 12:16:10, Michael Achenbach wrote: > Readability: > for arg in sys.argv[1:]: > # s/sys.argv[i]/arg Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:70: d8_exec = d8_line.groups()[0] On 2016/04/26 12:16:10, Michael Achenbach wrote: > d8_exec = d8_line.groups()[0] > equivalent to > d8_exec = d8_line.group(1) Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:73: exit(-1) On 2016/04/26 12:16:10, Michael Achenbach wrote: > Rather sys.exit(-1) in scripts I guess. Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:77: with open(log_file) as f: On 2016/04/26 12:16:10, Michael Achenbach wrote: > I'm not familiar with this pattern. Will it keep the logfile open until all data > is processed by sp.communicate()? What if the process is very slow? Wouldn't the > with clause close the file before it's exhausted? Sorry, I should put sp.communicate() in the clause. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:89: is_written = False if out else True On 2016/04/26 12:16:10, Michael Achenbach wrote: > Readability: > is_written = not out > > or if you prefer: > is_written = not bool(out) Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:90: with open(log_file, 'w+') as f: On 2016/04/26 12:16:10, Michael Achenbach wrote: > How about just w as you only write to the file? Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dump-cpp.py#newc... tools/dump-cpp.py:93: f.write(out) On 2016/04/26 12:16:10, Michael Achenbach wrote: > So you append all symbols right after the first tick? Yes. https://codereview.chromium.org/1884493003/diff/160001/tools/dumpcpp.js File tools/dumpcpp.js (right): https://codereview.chromium.org/1884493003/diff/160001/tools/dumpcpp.js#newcode1 tools/dumpcpp.js:1: // Copyright 2016 the V8 project authors. All rights reserved. On 2016/04/26 12:16:10, Michael Achenbach wrote: > nit: Use short copyright header for new files. Done. https://codereview.chromium.org/1884493003/diff/160001/tools/dumpcpp.js#newco... tools/dumpcpp.js:41: // Pull dev tools source maps into our name space. On 2016/04/26 12:52:51, Yang wrote: > weird whitespace between "maps" and "into" Done.
lgtm (mostly rubberstamp - checked code readability not if the tool does what it should)
On 2016/04/27 07:47:02, Michael Achenbach wrote: > lgtm (mostly rubberstamp - checked code readability not if the tool does what it > should) Thanks you. @Jarin, @Yang, could you please take a look? Thanks.
lgtm. https://codereview.chromium.org/1884493003/diff/180001/tools/dump-cpp.py File tools/dump-cpp.py (right): https://codereview.chromium.org/1884493003/diff/180001/tools/dump-cpp.py#newc... tools/dump-cpp.py:56: sp = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=f) Can we maintain the 80-char limit here? Thanks.
Thanks for the review. @Jarin, PTAL. https://codereview.chromium.org/1884493003/diff/180001/tools/dump-cpp.py File tools/dump-cpp.py (right): https://codereview.chromium.org/1884493003/diff/180001/tools/dump-cpp.py#newc... tools/dump-cpp.py:56: sp = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=f) On 2016/04/28 05:15:12, Yang wrote: > Can we maintain the 80-char limit here? Thanks. Done.
This was just FYI for jarin. I guess you can go ahead and land this for now. If there's anything you can add a follow up.
On 2016/04/28 06:25:54, Michael Achenbach wrote: > This was just FYI for jarin. I guess you can go ahead and land this for now. If > there's anything you can add a follow up. Thank you.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmeawad@chromium.org, yangguo@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1884493003/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1884493003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1884493003/200001
Message was sent while issue was closed.
Description was changed from
==========
Dump C++ symbols and merge into v8 log.
Currently we already have tools to extract C++ symbols of d8, and output the
statistics result. This patch creates two scripts, one is to use exsisting tools
to extract C++ symbols and dump to output, the other collects all symbols and
merges them into v8 log. The format of C++ symbols in v8 log is:
cpp,start_address,size,symbol_name
BUG=v8:4906
LOG=n
==========
to
==========
Dump C++ symbols and merge into v8 log.
Currently we already have tools to extract C++ symbols of d8, and output the
statistics result. This patch creates two scripts, one is to use exsisting tools
to extract C++ symbols and dump to output, the other collects all symbols and
merges them into v8 log. The format of C++ symbols in v8 log is:
cpp,start_address,size,symbol_name
BUG=v8:4906
LOG=n
==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from
==========
Dump C++ symbols and merge into v8 log.
Currently we already have tools to extract C++ symbols of d8, and output the
statistics result. This patch creates two scripts, one is to use exsisting tools
to extract C++ symbols and dump to output, the other collects all symbols and
merges them into v8 log. The format of C++ symbols in v8 log is:
cpp,start_address,size,symbol_name
BUG=v8:4906
LOG=n
==========
to
==========
Dump C++ symbols and merge into v8 log.
Currently we already have tools to extract C++ symbols of d8, and output the
statistics result. This patch creates two scripts, one is to use exsisting tools
to extract C++ symbols and dump to output, the other collects all symbols and
merges them into v8 log. The format of C++ symbols in v8 log is:
cpp,start_address,size,symbol_name
BUG=v8:4906
LOG=n
Committed: https://crrev.com/aff441937d6bb1e2869772414ac5f0b0e0480143
Cr-Commit-Position: refs/heads/master@{#35841}
==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/aff441937d6bb1e2869772414ac5f0b0e0480143 Cr-Commit-Position: refs/heads/master@{#35841} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
