Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
424973004:
Extend CPU profiler with mapping ticks to source lines (Closed)
DescriptionThe idea behind of this solution is to use the existing "relocation info" instead of consumption the CodeLinePosition events emitted by the V8 compilers.
During generation code and relocation info are generated simultaneously.
When code generation is done you each code object has associated "relocation info".
Relocation information lets V8 to mark interesting places in the generated code: the pointers that might need to be relocated (after garbage collection),
correspondences between the machine program counter and source locations for stack walking.
This patch:
1. Add more source positions info in reloc info to make it suitable for source level mapping.
The amount of data should not be increased dramatically because (1) V8 already marks interesting places in the generated code and
(2) V8 does not write redundant information (it writes a pair (pc_offset, pos) only if pos is changed and skips other).
I measured it on Octane benchmark - for unoptimized code the number of source positions may achieve 2x ('lin_solve' from NavierStokes benchmark).
2. When a sample happens, CPU profiler finds a code object by pc, then use its reloc info to match the sample to a source line.
If a source line is found that hit counter is increased by one for this line.
3. Add a new public V8 API to get the hit source lines by CDT CPU profiler.
Note that it's expected a minor patch in Blink to pack the source level info in JSON to be shown.
4.Add a test that checks how the samples are distributed through source lines.
It tests two cases: (1) relocation info created during code generation and (2) relocation info associated with precompiled function's version.
Patch from Denis Pravdin <denis.pravdin@intel.com>;
BUG=None
LOG=Y
Patch Set 1 #
Total comments: 25
Patch Set 2 : addressed the comments #
Total comments: 10
Patch Set 3 : Addressed comments #
Total comments: 13
Patch Set 4 : rebasing #Patch Set 5 : addressed comments #
Total comments: 13
Patch Set 6 : rebasing again #Patch Set 7 : addressed some comments #Patch Set 8 : addressed the various code object issue in the test #
Total comments: 4
Patch Set 9 : git cl format #
Total comments: 10
Patch Set 10 : addressed alph comments #Patch Set 11 : remove the change on cctest.status #Patch Set 12 : addressed yurys comments #Patch Set 13 : Addressed yurys comments #Patch Set 14 : rebasing #Patch Set 15 : Fixed TickLines failure when using turbofan after rebasing #Patch Set 16 : fixed v8_linux_nosnap_dbg failure #
Messages
Total messages: 60 (3 generated)
Hi Danno, This is a patch from Denis Pravdin <denis.pravdin@intel.com> to extend V8 CPU profiler by mapping ticks (samples) to the lines in appropriate source file. Could you please have a look? Thanks -Weiliang
Yury and Sven, could you please take a look? https://codereview.chromium.org/424973004/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/424973004/diff/1/AUTHORS#newcode31 AUTHORS:31: Denis Pravdin <denis.pravdin@intel.com> AUTHORs who are covered by a corporate CLA should not be individually added to the AUTHORS file (in this case, Intel's CLA covers). We will be updating the AUTHORs according to these guidelines soon, so please don't add new names. https://codereview.chromium.org/424973004/diff/1/src/log.h File src/log.h (right): https://codereview.chromium.org/424973004/diff/1/src/log.h#newcode162 src/log.h:162: int length = pc_offset_infos_.length(); Why not use binary_search/lower_bound here?
https://codereview.chromium.org/424973004/diff/1/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/424973004/diff/1/include/v8-profiler.h#newcode25 include/v8-profiler.h:25: unsigned int ticks; hitCount? https://codereview.chromium.org/424973004/diff/1/include/v8-profiler.h#newcode61 include/v8-profiler.h:61: * True if all available entries are copied, otherwise false. According to the implementation it copies nothing if buffer is not large enough. I think it should be stated more clearly here. https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc File src/cpu-profiler.cc (left): https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc#oldcode270 src/cpu-profiler.cc:270: ASSERT(Script::cast(shared->script())); why is it gone? https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc#newcode271 src/cpu-profiler.cc:271: if (position >= 0 && lineno > 0) { if position < 0 you don't need to make a call to GetLineNumber https://codereview.chromium.org/424973004/diff/1/src/log.h File src/log.h (right): https://codereview.chromium.org/424973004/diff/1/src/log.h#newcode163 src/log.h:163: if (0 == length) return 0; nit style: the v8 code doesn't seem to use const at the left in comparisons. https://codereview.chromium.org/424973004/diff/1/src/log.h#newcode214 src/log.h:214: pc_offset_infos_.Add(pc_offset_info); As long as the algo expects the entries are sorted by pc_offset, could you please add the corresponding ASSERT here to be on the safe side? https://codereview.chromium.org/424973004/diff/1/src/profile-generator-inl.h File src/profile-generator-inl.h (right): https://codereview.chromium.org/424973004/diff/1/src/profile-generator-inl.h#... src/profile-generator-inl.h:31: if (NULL != line_info) { style nit: NULL to the right hand side https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#new... src/profile-generator.cc:216: if (NULL != e) { It couldn't be NULL if insert arg is true. style: move const to the right hand side https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#new... src/profile-generator.cc:217: e->value = static_cast<char*>(e->value) + 1; why char*? uintptr_t? https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#new... src/profile-generator.cc:224: if (NULL == entries || number == 0) { ditto https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#new... src/profile-generator.cc:229: if (lineNumber == 0) return false; should return true. https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#new... src/profile-generator.cc:237: entry->line = reinterpret_cast<intptr_t>(p->key); entry->line is unsigned int, i.e. 32 bits. So it should produce a compile error on x64 (for sure on Win64). https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#new... src/profile-generator.cc:635: } could you please make it more readable, e.g.: int src_line = entry->line_info().GetSourceLineNumber(pc_offset); if (src_line) return src_line; return v8::CpuProfileNode::kNoLineNumberInfo;
https://codereview.chromium.org/424973004/diff/1/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/424973004/diff/1/include/v8-profiler.h#newcode20 include/v8-profiler.h:20: typedef struct { struct LineTick, also it can be nested in CpuProfileNode https://codereview.chromium.org/424973004/diff/1/include/v8-profiler.h#newcode22 include/v8-profiler.h:22: unsigned int line; Why only line number, column would also be interesting for minfied sources? Internally you could store map: position -> tick count and convert positions to (line, column) pairs when returning results. https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/1/src/cpu-profiler.cc#newcode262 src/cpu-profiler.cc:262: JITLineInfoTable line_table; Consider allocating JITLineInfoTable on the heap to avoid additional copy in CodeEntry constructor. https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#new... src/profile-generator.cc:217: e->value = static_cast<char*>(e->value) + 1; intptr_t https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#new... src/profile-generator.cc:379: int src_line) { It seems that with per line self tick count it would make sense to also collect per line total tick counts for each function so that the user can tell how many ticks we spent inside particular call. In other words we may need src_line for all frames not only the topmost one. This is a topic for a separate patch though. https://codereview.chromium.org/424973004/diff/1/src/profile-generator.cc#new... src/profile-generator.cc:691: bool unresolved_src = (src_line == v8::CpuProfileNode::kNoLineNumberInfo) ? bool unresolved_src = src_line == v8::CpuProfileNode::kNoLineNumberInfo; https://codereview.chromium.org/424973004/diff/1/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/424973004/diff/1/src/profile-generator.h#newc... src/profile-generator.h:161: int src_line = v8::CpuProfileNode::kNoLineNumberInfo); style: wrong alignment https://codereview.chromium.org/424973004/diff/1/src/profile-generator.h#newc... src/profile-generator.h:163: int src_line = v8::CpuProfileNode::kNoLineNumberInfo); ditto https://codereview.chromium.org/424973004/diff/1/test/cctest/test-cpu-profile... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/1/test/cctest/test-cpu-profile... test/cctest/test-cpu-profiler.cc:1090: delete [] entries; Can be handled using a smart pointer from src/smart-pointers.h https://codereview.chromium.org/424973004/diff/1/test/cctest/test-cpu-profile... test/cctest/test-cpu-profiler.cc:1185: TEST(TickLines) { The test is bound to be flaky like other CPU profiler tests that rely on statistical data. We need to use something more predictable than a real profile data collected during test run. Probably some mocked samples or something else. The tests are already marked 'test-cpu-profiler/*': [PASS, FLAKY], so please don't add one more that would very likely fail time to time.
LGTM from my side if the previous comments are addressed.
Thanks a lot for all of your reviews. The comments are addressed and please take a look the patch set 2.
On 2014/08/06 04:10:16, Weiliang wrote: > Thanks a lot for all of your reviews. The comments are addressed and please take > a look the patch set 2. Hi Danno, Yuri and Sven, Could please take a look at the patch set 2. I reworked the test to make it more deterministic and addressed some of your comments. Thanks!
LGTM from my side (still), but we should wait for Yury, too...
https://codereview.chromium.org/424973004/diff/20001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/424973004/diff/20001/include/v8-profiler.h#ne... include/v8-profiler.h:25: typedef struct { style: we use C++ style for struct declaration. struct LineTick { ... } https://codereview.chromium.org/424973004/diff/20001/include/v8-profiler.h#ne... include/v8-profiler.h:27: int line; How about providing column as well which I already suggested before? On minified sources line number doesn't give enough information as the whole script may be formatted as one line. https://codereview.chromium.org/424973004/diff/20001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/20001/src/profile-generator.cc... src/profile-generator.cc:231: unsigned int number) const { nit: number -> length https://codereview.chromium.org/424973004/diff/20001/src/profile-generator.cc... src/profile-generator.cc:234: unsigned line_number = line_ticks_.occupancy(); line_count https://codereview.chromium.org/424973004/diff/20001/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/424973004/diff/20001/src/profile-generator.h#... src/profile-generator.h:162: unsigned int number) const; nit:number -> length https://codereview.chromium.org/424973004/diff/20001/test/cctest/test-cpu-pro... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/20001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:1074: "Logger::LogCompiledFunctions"); "GetFunctionCodeFromHeap" https://codereview.chromium.org/424973004/diff/20001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:1123: "})(this)\n", foo_name); 'this' passed as timeout looks like an error. https://codereview.chromium.org/424973004/diff/20001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:1139: v8::base::OS::Sleep(100); // Ensure that a new node is added to the profile. We need to use some explicit event instead of Sleep. I believe calling profiler->StopProfiling below should be enough as it should trigger processing of all enqueued events. https://codereview.chromium.org/424973004/diff/20001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:1143: CodeEntry* foo = generator->code_map()->FindEntry(address); This is not thread-safe as code_map is used on the processor thread. It should be enough to add a few fake events into the queue, stop profiling and check that resulting tree reflects corresponding hit counts.
https://codereview.chromium.org/424973004/diff/20001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/424973004/diff/20001/include/v8-profiler.h#ne... include/v8-profiler.h:27: int line; On 2014/08/08 08:13:23, yurys wrote: > How about providing column as well which I already suggested before? On minified > sources line number doesn't give enough information as the whole script may be > formatted as one line. The minified sources is useful to reduce the amount data that needs to be transferred. It's difficult and inconvenient to use them for profiling purpose. When developer identifies a position (column in alone line representing his script) that impacts performance that next step is modification of source code. I'm not sure that the minified sources give a convenient way to understand a logic of the algorithm implemented in a function, it's better to use original version of source file. That's why I would suggest not to provide column in this patch. We always can do that if the customers find it useful for profiling. What do you think?
On 2014/08/11 05:35:31, Denis Pravdin wrote: > https://codereview.chromium.org/424973004/diff/20001/include/v8-profiler.h > File include/v8-profiler.h (right): > > https://codereview.chromium.org/424973004/diff/20001/include/v8-profiler.h#ne... > include/v8-profiler.h:27: int line; > On 2014/08/08 08:13:23, yurys wrote: > > How about providing column as well which I already suggested before? On > minified > > sources line number doesn't give enough information as the whole script may be > > formatted as one line. > > The minified sources is useful to reduce the amount data that needs to be > transferred. It's difficult and inconvenient to use them for profiling purpose. > When developer identifies a position (column in alone line representing his > script) that impacts performance that next step is modification of source code. > I'm not sure that the minified sources give a convenient way to understand a > logic of the algorithm implemented in a function, it's better to use original > version of source file. That's why I would suggest not to provide column in this > patch. We always can do that if the customers find it useful for profiling. What > do you think? We do have customers (inside Google) who profile sources produced by closure compiler. Also having a line+column we can use source maps to show position in the original source code while profiling compiled scripts. It won't be possible without column number. Copmplied scripts + source maps is quite common use case, that's why I suggested we should provide column from the very beginning especially given that it shouldn't complicate the implementation. This can be implemented in a separate patch if you feel that it would be more convenient by I think we need to support column information as well.
On 2014/08/11 06:19:52, yurys wrote: > On 2014/08/11 05:35:31, Denis Pravdin wrote: > > https://codereview.chromium.org/424973004/diff/20001/include/v8-profiler.h > > File include/v8-profiler.h (right): > > > > > https://codereview.chromium.org/424973004/diff/20001/include/v8-profiler.h#ne... > > include/v8-profiler.h:27: int line; > > On 2014/08/08 08:13:23, yurys wrote: > > > How about providing column as well which I already suggested before? On > > minified > > > sources line number doesn't give enough information as the whole script may > be > > > formatted as one line. > > > > The minified sources is useful to reduce the amount data that needs to be > > transferred. It's difficult and inconvenient to use them for profiling > purpose. > > When developer identifies a position (column in alone line representing his > > script) that impacts performance that next step is modification of source > code. > > I'm not sure that the minified sources give a convenient way to understand a > > logic of the algorithm implemented in a function, it's better to use original > > version of source file. That's why I would suggest not to provide column in > this > > patch. We always can do that if the customers find it useful for profiling. > What > > do you think? > > We do have customers (inside Google) who profile sources produced by closure > compiler. Also having a line+column we can use source maps to show position in > the original source code while profiling compiled scripts. It won't be possible > without column number. Copmplied scripts + source maps is quite common use case, > that's why I suggested we should provide column from the very beginning > especially given that it shouldn't complicate the implementation. This can be > implemented in a separate patch if you feel that it would be more convenient by > I think we need to support column information as well. Thank you for letting me know about this usage case. Now I see that we need to support column information. Please do that in a separate patch.
On behalf of Denis Please take a look at the patch set 3. Thanks a lot
https://codereview.chromium.org/424973004/diff/40001/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/40001/src/cpu-profiler.cc#newc... src/cpu-profiler.cc:260: ASSERT(Script::cast(shared->script())); nit: DCHECK here and everywhere else. Also cast does SLOW_DCHECK internally. Anyway if you still want to do the check here, it's better to use: DCHECK(shared->script()->IsScript()); https://codereview.chromium.org/424973004/diff/40001/src/cpu-profiler.cc#newc... src/cpu-profiler.cc:265: Handle<Script> script_handle(Script::cast(shared->script())); nit: you can use the 'script' pointer calculated above. https://codereview.chromium.org/424973004/diff/40001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/40001/src/profile-generator.cc... src/profile-generator.cc:226: e->value = static_cast<char*>(e->value) + 1; should be cast to uintptr_t https://codereview.chromium.org/424973004/diff/40001/src/profile-generator.cc... src/profile-generator.cc:244: entry->line = reinterpret_cast<uintptr_t>(p->key); I still bet it won't compile on x64 because of implicit 64->32 bits conversion. You have to write something like: static_cast<usigned int>(reinterpret_cast<uintptr_t>(p->key)); https://codereview.chromium.org/424973004/diff/40001/test/cctest/test-cpu-pro... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/40001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:1093: if (strcmp(str.get(), name) == 0) It's better to make a v8 heap string out of 'name' using v8::String::NewFromUtf8 so you can compare two heap strings instead. This way you won't need to flatten each string in the heap to make the comparison. https://codereview.chromium.org/424973004/diff/40001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:1116: " while (Date.now() - start < timeout) {\n" This test bound to be flaky as it depends on the timeouts. Instead if you want the foo function surely got into profile, make a call to startProfiling right out of foo. startProfiling always takes the first sample at its invocation point. See BoundFunctionCall test for example. https://codereview.chromium.org/424973004/diff/40001/test/cctest/test-cpu-pro... test/cctest/test-cpu-profiler.cc:1129: i::SharedFunctionInfo* shared = GetFunctionInfoFromHeap(isolate, foo_name); You can get the JSFunction object out of global context after CompileRun instead of iterating the whole heap. See CreateCode function body above for example.
https://codereview.chromium.org/424973004/diff/40001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/40001/src/profile-generator.cc... src/profile-generator.cc:226: e->value = static_cast<char*>(e->value) + 1; On 2014/08/11 12:18:15, alph wrote: > should be cast to uintptr_t e->value is type of void* and used as a hit counter. To increment it by 1 void* should be casted to char* but not uintptr*. otherwise it will be incremented by sizeof(int). https://codereview.chromium.org/424973004/diff/40001/src/profile-generator.cc... src/profile-generator.cc:244: entry->line = reinterpret_cast<uintptr_t>(p->key); On 2014/08/11 12:18:15, alph wrote: > I still bet it won't compile on x64 because of implicit 64->32 bits conversion. > You have to write something like: > static_cast<usigned int>(reinterpret_cast<uintptr_t>(p->key)); Acknowledged.
https://codereview.chromium.org/424973004/diff/40001/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/40001/src/cpu-profiler.cc#newc... src/cpu-profiler.cc:260: ASSERT(Script::cast(shared->script())); On 2014/08/11 12:18:15, alph wrote: > nit: DCHECK here and everywhere else. > > Also cast does SLOW_DCHECK internally. > Anyway if you still want to do the check here, it's better to use: > DCHECK(shared->script()->IsScript()); DCHECK is undefined. Do you suggest to use the CHECK macro? https://codereview.chromium.org/424973004/diff/40001/src/cpu-profiler.cc#newc... src/cpu-profiler.cc:265: Handle<Script> script_handle(Script::cast(shared->script())); On 2014/08/11 12:18:15, alph wrote: > nit: you can use the 'script' pointer calculated above. Acknowledged.
https://codereview.chromium.org/424973004/diff/40001/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/40001/src/cpu-profiler.cc#newc... src/cpu-profiler.cc:260: ASSERT(Script::cast(shared->script())); On 2014/08/11 13:11:50, Denis Pravdin wrote: > On 2014/08/11 12:18:15, alph wrote: > > nit: DCHECK here and everywhere else. > > > > Also cast does SLOW_DCHECK internally. > > Anyway if you still want to do the check here, it's better to use: > > DCHECK(shared->script()->IsScript()); > > DCHECK is undefined. Do you suggest to use the CHECK macro? Rebaseline to the latest version. They've been introduced a couple weeks ago. https://codereview.chromium.org/424973004/diff/40001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/40001/src/profile-generator.cc... src/profile-generator.cc:226: e->value = static_cast<char*>(e->value) + 1; On 2014/08/11 13:00:13, Denis Pravdin wrote: > On 2014/08/11 12:18:15, alph wrote: > > should be cast to uintptr_t > > e->value is type of void* and used as a hit counter. To increment it by 1 void* > should be casted to char* but not uintptr*. otherwise it will be incremented by > sizeof(int). I didn't say uintptr*, I said uintptr_t :-) uintptr_t is an integer type having a size of the platform pointer. The cast to char* does also work, but it looks like a trick relying on the fact sizeof(char) is 1. I meant to use the following snippet to be strictly correct with types: e->value = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(e->value) + 1);
rebased + addressed comments PTAL
On 2014/08/13 08:55:39, Weiliang wrote: > rebased + addressed comments > PTAL Please take a look at the patch. Thanks.
https://codereview.chromium.org/424973004/diff/120001/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/120001/src/cpu-profiler.cc#new... src/cpu-profiler.cc:261: if (shared->script()->IsScript()) { Is it possible the shared->script() is not a Script? I see previously it was not the case. https://codereview.chromium.org/424973004/diff/120001/src/cpu-profiler.cc#new... src/cpu-profiler.cc:263: script = Script::cast(shared->script()); Script::cast does the DCHECK internally so you don't really need to do it here. Anyway if you still want to double check, could you please write: script = Script::cast(shared->script()); DCHECK(script); https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h... src/profile-generator.h:58: pc_offset_map_.insert(std::make_pair(pc_offset, line)); a possible optimization: you don't need to bloat it with a new entry if lower_bound for the pc_offset does already return line. https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1134: CHECK_NE(NULL, line_info); I've got the test failing at this line for ia32 target because there's no POSITION reloc infos for the code.
https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1134: CHECK_NE(NULL, line_info); On 2014/08/15 12:10:47, alph wrote: > I've got the test failing at this line for ia32 target because there's no > POSITION reloc infos for the code. Well, I found out what causes this test failure but I don’t understand a reason of this behavior and I am not sure what the best way to fix it is: In patch set 3, I iterate the heap to get a function object (GetFunctionInfoFromHeap). The test passes all the time. In patch set 4, I replace it with the lines below as you suggested (see a code snippet below) and test failed. i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( *v8::Local<v8::Function>::Cast( (*env)->Global()->Get(v8_str(func_name)))); Looking into it, I decided to check what the difference between these two ways: 1. I added heap iteration back in addition to v8::Utils::OpenHandle 2. Then checked that reloc info for both objects of Code type include positions. The test looks like: CompileRun(script.start()); i::SharedFunctionInfo* shared = GetFunctionInfoFromHeap(isolate, func_name); int positions_ = 0; for (i::RelocIterator it(shared->code()); !it.done(); it.next()) { i::RelocInfo::Mode mode = it.rinfo()->rmode(); if (i::RelocInfo::IsPosition(mode)) positions_++; } CHECK_GT(positions_, 0); // THIS CHECK PASSED i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( *v8::Local<v8::Function>::Cast( (*env)->Global()->Get(v8_str(func_name)))); CHECK_NE(NULL, func->shared()); CHECK_NE(NULL, func->code()); int positions = 0; for (i::RelocIterator it(func->code()); !it.done(); it.next()) { i::RelocInfo::Mode mode = it.rinfo()->rmode(); if (i::RelocInfo::IsPosition(mode)) positions++; } CHECK_GT(positions, 0); // THIS CHECK DOES NOT PASS. Do you have idea why for the first case I have POSITION reloc info, and never have it for the second case? It seems it's Linux specific issue. At least I don't see it on Windows.
https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1134: CHECK_NE(NULL, line_info); On 2014/08/15 20:57:55, Denis Pravdin wrote: > On 2014/08/15 12:10:47, alph wrote: > > I've got the test failing at this line for ia32 target because there's no > > POSITION reloc infos for the code. > > Well, I found out what causes this test failure but I don’t understand a reason > of this behavior and I am not sure what the best way to fix it is: > > In patch set 3, I iterate the heap to get a function object > (GetFunctionInfoFromHeap). The test passes all the time. > > In patch set 4, I replace it with the lines below as you suggested (see a code > snippet below) and test failed. > > i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( > *v8::Local<v8::Function>::Cast( > (*env)->Global()->Get(v8_str(func_name)))); > > Looking into it, I decided to check what the difference between these two ways: > 1. I added heap iteration back in addition to v8::Utils::OpenHandle > 2. Then checked that reloc info for both objects of Code type include positions. > > The test looks like: > > CompileRun(script.start()); > > i::SharedFunctionInfo* shared = GetFunctionInfoFromHeap(isolate, func_name); > int positions_ = 0; > for (i::RelocIterator it(shared->code()); !it.done(); it.next()) { > i::RelocInfo::Mode mode = it.rinfo()->rmode(); > if (i::RelocInfo::IsPosition(mode)) positions_++; > } > CHECK_GT(positions_, 0); // THIS CHECK PASSED > > i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( > *v8::Local<v8::Function>::Cast( > (*env)->Global()->Get(v8_str(func_name)))); > CHECK_NE(NULL, func->shared()); > CHECK_NE(NULL, func->code()); > int positions = 0; > for (i::RelocIterator it(func->code()); !it.done(); it.next()) { > i::RelocInfo::Mode mode = it.rinfo()->rmode(); > if (i::RelocInfo::IsPosition(mode)) positions++; > } > CHECK_GT(positions, 0); // THIS CHECK DOES NOT PASS. > > Do you have idea why for the first case I have POSITION reloc info, and never > have it for the second case? Could you please check the 'shared' and 'code' objects you get using these two approaches are the same. I think there are several versions on the code for the function, e.g. optimized and non-optimized. One of them is missing relocations. > > It seems it's Linux specific issue. At least I don't see it on Windows.
On 2014/08/19 14:23:35, alph wrote: > https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... > File test/cctest/test-cpu-profiler.cc (right): > > https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... > test/cctest/test-cpu-profiler.cc:1134: CHECK_NE(NULL, line_info); > On 2014/08/15 20:57:55, Denis Pravdin wrote: > > On 2014/08/15 12:10:47, alph wrote: > > > I've got the test failing at this line for ia32 target because there's no > > > POSITION reloc infos for the code. > > > > Well, I found out what causes this test failure but I don’t understand a > reason > > of this behavior and I am not sure what the best way to fix it is: > > > > In patch set 3, I iterate the heap to get a function object > > (GetFunctionInfoFromHeap). The test passes all the time. > > > > In patch set 4, I replace it with the lines below as you suggested (see a code > > snippet below) and test failed. > > > > i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( > > *v8::Local<v8::Function>::Cast( > > (*env)->Global()->Get(v8_str(func_name)))); > > > > Looking into it, I decided to check what the difference between these two > ways: > > 1. I added heap iteration back in addition to v8::Utils::OpenHandle > > 2. Then checked that reloc info for both objects of Code type include > positions. > > > > The test looks like: > > > > CompileRun(script.start()); > > > > i::SharedFunctionInfo* shared = GetFunctionInfoFromHeap(isolate, func_name); > > int positions_ = 0; > > for (i::RelocIterator it(shared->code()); !it.done(); it.next()) { > > i::RelocInfo::Mode mode = it.rinfo()->rmode(); > > if (i::RelocInfo::IsPosition(mode)) positions_++; > > } > > CHECK_GT(positions_, 0); // THIS CHECK PASSED > > > > i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( > > *v8::Local<v8::Function>::Cast( > > (*env)->Global()->Get(v8_str(func_name)))); > > CHECK_NE(NULL, func->shared()); > > CHECK_NE(NULL, func->code()); > > int positions = 0; > > for (i::RelocIterator it(func->code()); !it.done(); it.next()) { > > i::RelocInfo::Mode mode = it.rinfo()->rmode(); > > if (i::RelocInfo::IsPosition(mode)) positions++; > > } > > CHECK_GT(positions, 0); // THIS CHECK DOES NOT PASS. > > > > Do you have idea why for the first case I have POSITION reloc info, and never > > have it for the second case? > > Could you please check the 'shared' and 'code' objects you get using these two > approaches are the same. > > I think there are several versions on the code for the function, e.g. optimized > and non-optimized. One of them is missing relocations. > Yes, you're right. I debugged the test and found out: 1. There are two ways to get 'code' using JSFunction object (jsf is type of JSFunction): (a) jsf->code() (b) jsf->shared()->code() 2. When optimized code is compiled that both JSFunction::ReplaceCode(Code* code) and SharedFunctionInfo::ReplaceCode(Code* value) function are called. As the result, the 'code' objects returned by (a) and (b) contains relocations. 3. Then, optimized version of code is generated and only SFunction::ReplaceCode(Code* code) is called. Note that I use func->code() instead of func->shared()->code(): for (i::RelocIterator it(func->code()); !it.done(); it.next()) { i::RelocInfo::Mode mode = it.rinfo()->rmode(); if (i::RelocInfo::IsPosition(mode)) positions++; } That's why my previous approach (GetFunctionInfoFromHeap) always works but new one (OpenHandle) causes a test failure. Possible solutions: 1) For testing purpose, always use unoptimized version of code (e.g. func->shared()->code()) + allows to make the test more deterministic - optimized code is not tested 2) Set FLAG_hydrogen_track_positions flag as true by default before CompileRun for this test. I checked on Windows and see that number of positions is increased from 9 to 19 for 'func' function from the test. I think that the flag should be turned on always to achieve better mapping to source lines for generated optimized code. We can add a runtime switch to turn it on before profiling starts. It's a subject for a separate patch. Can you suggest other ideas? If not that which of the above is the best? > > > > It seems it's Linux specific issue. At least I don't see it on Windows.
On 2014/08/19 14:23:35, alph wrote: > https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... > File test/cctest/test-cpu-profiler.cc (right): > > https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... > test/cctest/test-cpu-profiler.cc:1134: CHECK_NE(NULL, line_info); > On 2014/08/15 20:57:55, Denis Pravdin wrote: > > On 2014/08/15 12:10:47, alph wrote: > > > I've got the test failing at this line for ia32 target because there's no > > > POSITION reloc infos for the code. > > > > Well, I found out what causes this test failure but I don’t understand a > reason > > of this behavior and I am not sure what the best way to fix it is: > > > > In patch set 3, I iterate the heap to get a function object > > (GetFunctionInfoFromHeap). The test passes all the time. > > > > In patch set 4, I replace it with the lines below as you suggested (see a code > > snippet below) and test failed. > > > > i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( > > *v8::Local<v8::Function>::Cast( > > (*env)->Global()->Get(v8_str(func_name)))); > > > > Looking into it, I decided to check what the difference between these two > ways: > > 1. I added heap iteration back in addition to v8::Utils::OpenHandle > > 2. Then checked that reloc info for both objects of Code type include > positions. > > > > The test looks like: > > > > CompileRun(script.start()); > > > > i::SharedFunctionInfo* shared = GetFunctionInfoFromHeap(isolate, func_name); > > int positions_ = 0; > > for (i::RelocIterator it(shared->code()); !it.done(); it.next()) { > > i::RelocInfo::Mode mode = it.rinfo()->rmode(); > > if (i::RelocInfo::IsPosition(mode)) positions_++; > > } > > CHECK_GT(positions_, 0); // THIS CHECK PASSED > > > > i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( > > *v8::Local<v8::Function>::Cast( > > (*env)->Global()->Get(v8_str(func_name)))); > > CHECK_NE(NULL, func->shared()); > > CHECK_NE(NULL, func->code()); > > int positions = 0; > > for (i::RelocIterator it(func->code()); !it.done(); it.next()) { > > i::RelocInfo::Mode mode = it.rinfo()->rmode(); > > if (i::RelocInfo::IsPosition(mode)) positions++; > > } > > CHECK_GT(positions, 0); // THIS CHECK DOES NOT PASS. > > > > Do you have idea why for the first case I have POSITION reloc info, and never > > have it for the second case? > > Could you please check the 'shared' and 'code' objects you get using these two > approaches are the same. > > I think there are several versions on the code for the function, e.g. optimized > and non-optimized. One of them is missing relocations. > Yes, you're right. I debugged the test and found out: 1. There are two ways to get 'code' using JSFunction object (jsf is type of JSFunction): (a) jsf->code() (b) jsf->shared()->code() 2. When optimized code is compiled that both JSFunction::ReplaceCode(Code* code) and SharedFunctionInfo::ReplaceCode(Code* value) function are called. As the result, the 'code' objects returned by (a) and (b) contains relocations. 3. Then, optimized version of code is generated and only SFunction::ReplaceCode(Code* code) is called. Note that I use func->code() instead of func->shared()->code(): for (i::RelocIterator it(func->code()); !it.done(); it.next()) { i::RelocInfo::Mode mode = it.rinfo()->rmode(); if (i::RelocInfo::IsPosition(mode)) positions++; } That's why my previous approach (GetFunctionInfoFromHeap) always works but new one (OpenHandle) causes a test failure. Possible solutions: 1) For testing purpose, always use unoptimized version of code (e.g. func->shared()->code()) + allows to make the test more deterministic - optimized code is not tested 2) Set FLAG_hydrogen_track_positions flag as true by default before CompileRun for this test. I checked on Windows and see that number of positions is increased from 9 to 19 for 'func' function from the test. I think that the flag should be turned on always to achieve better mapping to source lines for generated optimized code. We can add a runtime switch to turn it on before profiling starts. It's a subject for a separate patch. Can you suggest other ideas? If not that which of the above is the best? > > > > It seems it's Linux specific issue. At least I don't see it on Windows.
https://codereview.chromium.org/424973004/diff/120001/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/120001/src/cpu-profiler.cc#new... src/cpu-profiler.cc:261: if (shared->script()->IsScript()) { On 2014/08/15 12:10:47, alph wrote: > Is it possible the shared->script() is not a Script? I see previously it was not > the case. I reused the same pattern as CpuProfiler::CodeCreateEvent(Logger::LogEventsAndTags tag, Code* code, SharedFunctionInfo* shared, CompilationInfo* info, Name* script_name) has. It does the same check. Is it OK?
On 2014/08/22 17:20:58, Denis Pravdin wrote: > On 2014/08/19 14:23:35, alph wrote: > > > https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... > > File test/cctest/test-cpu-profiler.cc (right): > > > > > https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... > > test/cctest/test-cpu-profiler.cc:1134: CHECK_NE(NULL, line_info); > > On 2014/08/15 20:57:55, Denis Pravdin wrote: > > > On 2014/08/15 12:10:47, alph wrote: > > > > I've got the test failing at this line for ia32 target because there's no > > > > POSITION reloc infos for the code. > > > > > > Well, I found out what causes this test failure but I don’t understand a > > reason > > > of this behavior and I am not sure what the best way to fix it is: > > > > > > In patch set 3, I iterate the heap to get a function object > > > (GetFunctionInfoFromHeap). The test passes all the time. > > > > > > In patch set 4, I replace it with the lines below as you suggested (see a > code > > > snippet below) and test failed. > > > > > > i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( > > > *v8::Local<v8::Function>::Cast( > > > (*env)->Global()->Get(v8_str(func_name)))); > > > > > > Looking into it, I decided to check what the difference between these two > > ways: > > > 1. I added heap iteration back in addition to v8::Utils::OpenHandle > > > 2. Then checked that reloc info for both objects of Code type include > > positions. > > > > > > The test looks like: > > > > > > CompileRun(script.start()); > > > > > > i::SharedFunctionInfo* shared = GetFunctionInfoFromHeap(isolate, > func_name); > > > int positions_ = 0; > > > for (i::RelocIterator it(shared->code()); !it.done(); it.next()) { > > > i::RelocInfo::Mode mode = it.rinfo()->rmode(); > > > if (i::RelocInfo::IsPosition(mode)) positions_++; > > > } > > > CHECK_GT(positions_, 0); // THIS CHECK PASSED > > > > > > i::Handle<i::JSFunction> func = v8::Utils::OpenHandle( > > > *v8::Local<v8::Function>::Cast( > > > (*env)->Global()->Get(v8_str(func_name)))); > > > CHECK_NE(NULL, func->shared()); > > > CHECK_NE(NULL, func->code()); > > > int positions = 0; > > > for (i::RelocIterator it(func->code()); !it.done(); it.next()) { > > > i::RelocInfo::Mode mode = it.rinfo()->rmode(); > > > if (i::RelocInfo::IsPosition(mode)) positions++; > > > } > > > CHECK_GT(positions, 0); // THIS CHECK DOES NOT PASS. > > > > > > Do you have idea why for the first case I have POSITION reloc info, and > never > > > have it for the second case? > > > > Could you please check the 'shared' and 'code' objects you get using these two > > approaches are the same. > > > > I think there are several versions on the code for the function, e.g. > optimized > > and non-optimized. One of them is missing relocations. > > > Yes, you're right. > I debugged the test and found out: > 1. There are two ways to get 'code' using JSFunction object (jsf is type of > JSFunction): > (a) jsf->code() > (b) jsf->shared()->code() > 2. When optimized code is compiled that both JSFunction::ReplaceCode(Code* code) > and SharedFunctionInfo::ReplaceCode(Code* value) function are called. As the > result, the 'code' objects returned by (a) and (b) contains relocations. > 3. Then, optimized version of code is generated and only > SFunction::ReplaceCode(Code* code) is called. > Note that I use func->code() instead of func->shared()->code(): > > for (i::RelocIterator it(func->code()); !it.done(); it.next()) { > i::RelocInfo::Mode mode = it.rinfo()->rmode(); > if (i::RelocInfo::IsPosition(mode)) positions++; > } > > That's why my previous approach (GetFunctionInfoFromHeap) always works but new > one (OpenHandle) causes a test failure. > > Possible solutions: > > 1) For testing purpose, always use unoptimized version of code (e.g. > func->shared()->code()) > + allows to make the test more deterministic > - optimized code is not tested > > 2) Set FLAG_hydrogen_track_positions flag as true by default before CompileRun > for this test. > I checked on Windows and see that number of positions is increased from 9 to > 19 for 'func' function from the test. > > I think that the flag should be turned on always to achieve better mapping to > source lines for generated optimized code. We can add a runtime switch to turn > it on before profiling starts. It's a subject for a separate patch. > > Can you suggest other ideas? If not that which of the above is the best? > > > > > > It seems it's Linux specific issue. At least I don't see it on Windows. Any comments?
https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/120001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1134: CHECK_NE(NULL, line_info); Yes, you're right. I debugged the test and found out: 1. There are two ways to get 'code' using JSFunction object (jsf is type of JSFunction): (a) jsf->code() (b) jsf->shared()->code() 2. When optimized code is compiled that both JSFunction::ReplaceCode(Code* code)and SharedFunctionInfo::ReplaceCode(Code* value) function are called. As the result, the 'code' objects returned by (a) and (b) contains relocations. 3. Then, optimized version of code is generated and only SFunction::ReplaceCode(Code* code) is called. Note that I use func->code() instead of func->shared()->code(): for (i::RelocIterator it(func->code()); !it.done(); it.next()) { i::RelocInfo::Mode mode = it.rinfo()->rmode(); if (i::RelocInfo::IsPosition(mode)) positions++; } That's why my previous approach (GetFunctionInfoFromHeap) always works but new one (OpenHandle) causes a test failure. Possible solutions: 1) For testing purpose, always use unoptimized version of code (e.g. func->shared()->code()) + allows to make the test more deterministic - optimized code is not tested 2) Set FLAG_hydrogen_track_positions flag as true by default before CompileRun for this test. I checked on Windows and see that number of positions is increased from 9 to 19 for 'func' function from the test. I think that the flag should be turned on always to achieve better mapping to source lines for generated optimized code. We can add a runtime switch to turn it on before profiling starts. It's a subject for a separate patch. Can you suggest other ideas? If not that which of the above is the best?
https://codereview.chromium.org/424973004/diff/120001/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/120001/src/cpu-profiler.cc#new... src/cpu-profiler.cc:261: if (shared->script()->IsScript()) { I reused the same pattern as CpuProfiler::CodeCreateEvent(Logger::LogEventsAndTags tag, Code* code, SharedFunctionInfo* shared, CompilationInfo* info, Name* script_name) has. It does the same check. Is it OK? On 2014/08/15 12:10:47, alph wrote: > Is it possible the shared->script() is not a Script? I see previously it was not > the case. https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h... src/profile-generator.h:58: pc_offset_map_.insert(std::make_pair(pc_offset, line)); SetPosition is called during code generation. GetSourceLineNumber is called after the code is generated and line info table is built. Could you please clarify what kind of optimization are your talking about? On 2014/08/15 12:10:47, alph wrote: > a possible optimization: you don't need to bloat it with a new entry if > lower_bound for the pc_offset does already return line.
https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h... src/profile-generator.h:58: pc_offset_map_.insert(std::make_pair(pc_offset, line)); On 2014/09/01 10:44:38, Denis Pravdin wrote: > SetPosition is called during code generation. GetSourceLineNumber is called > after the code is generated and line info table is built. Could you please > clarify what kind of optimization are your talking about? > > On 2014/08/15 12:10:47, alph wrote: > > a possible optimization: you don't need to bloat it with a new entry if > > lower_bound for the pc_offset does already return line. > Does it mean below? if (GetSourceLineNumber(pc_offset) == v8::CpuProfileNode::kNoLineNumberInfo) { pc_offset_map_.insert(std::make_pair(pc_offset, line)); }
On 2014/09/03 02:29:48, Weiliang wrote: > https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h > File src/profile-generator.h (right): > > https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h... > src/profile-generator.h:58: pc_offset_map_.insert(std::make_pair(pc_offset, > line)); > On 2014/09/01 10:44:38, Denis Pravdin wrote: > > SetPosition is called during code generation. GetSourceLineNumber is called > > after the code is generated and line info table is built. Could you please > > clarify what kind of optimization are your talking about? > > > > On 2014/08/15 12:10:47, alph wrote: > > > a possible optimization: you don't need to bloat it with a new entry if > > > lower_bound for the pc_offset does already return line. > > > > Does it mean below? > if (GetSourceLineNumber(pc_offset) == > v8::CpuProfileNode::kNoLineNumberInfo) { > pc_offset_map_.insert(std::make_pair(pc_offset, line)); > } Hi alph, danno and other reviewers Could you please have a look? The patch set 8 addressed the various codes issue. The previous failure is because the function is already marked to be optimized but before the OSR attempt, the func->code() is set to be the builtin of kCompileOptimized*, which is not the code object we want to test. The latest one adds some logic to get the correct code based on current function's optimization status. BTW, it seems that test/cctest/test-cpu-profiler is disable by the r23130(https://codereview.chromium.org/473873002). We enable it for our local test. Thanks again for your review. Thanks -Weiliang
On 2014/09/03 08:34:50, Weiliang wrote: > On 2014/09/03 02:29:48, Weiliang wrote: > > https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h > > File src/profile-generator.h (right): > > > > > https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h... > > src/profile-generator.h:58: pc_offset_map_.insert(std::make_pair(pc_offset, > > line)); > > On 2014/09/01 10:44:38, Denis Pravdin wrote: > > > SetPosition is called during code generation. GetSourceLineNumber is called > > > after the code is generated and line info table is built. Could you please > > > clarify what kind of optimization are your talking about? > > > > > > On 2014/08/15 12:10:47, alph wrote: > > > > a possible optimization: you don't need to bloat it with a new entry if > > > > lower_bound for the pc_offset does already return line. > > > > > > > Does it mean below? > > if (GetSourceLineNumber(pc_offset) == > > v8::CpuProfileNode::kNoLineNumberInfo) { > > pc_offset_map_.insert(std::make_pair(pc_offset, line)); > > } > > Hi alph, danno and other reviewers > Could you please have a look? > > The patch set 8 addressed the various codes issue. The previous failure is > because the function is already marked to be optimized but before the OSR > attempt, the func->code() is set to be the builtin of kCompileOptimized*, which > is not the code object we want to test. The latest one adds some logic to get > the correct code based on current function's optimization status. > > BTW, it seems that test/cctest/test-cpu-profiler is disable by the > r23130(https://codereview.chromium.org/473873002). We enable it for our local > test. Thanks again for your review. > > Thanks > -Weiliang Kindly ping. Hope it will not be rotted. :) Thanks -Weiliang
Sorry for long delay. lgtm. Please make sure you get one from yurys@ Thank you! https://codereview.chromium.org/424973004/diff/120001/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/120001/src/cpu-profiler.cc#new... src/cpu-profiler.cc:261: if (shared->script()->IsScript()) { On 2014/08/23 05:18:52, Denis Pravdin wrote: > On 2014/08/15 12:10:47, alph wrote: > > Is it possible the shared->script() is not a Script? I see previously it was > not > > the case. > > I reused the same pattern as > CpuProfiler::CodeCreateEvent(Logger::LogEventsAndTags tag, Code* code, > SharedFunctionInfo* shared, CompilationInfo* info, Name* script_name) has. > It does the same check. Is it OK? > That function is called from a different place, so it may have different assumptions about script(). This function does always have script() pointing to the correct Script object, so the check is not necessary. https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/424973004/diff/120001/src/profile-generator.h... src/profile-generator.h:58: pc_offset_map_.insert(std::make_pair(pc_offset, line)); I meant: if (GetSourceLineNumber(pc_offset) != line) { pc_offset_map_.insert(std::make_pair(pc_offset, line)); } https://codereview.chromium.org/424973004/diff/180001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/180001/src/profile-generator.c... src/profile-generator.cc:219: line_ticks_.Lookup(reinterpret_cast<void*>(src_line), src_line, true); bad padding. Could you please run it through 'git cl format' https://codereview.chromium.org/424973004/diff/180001/src/profile-generator.c... src/profile-generator.cc:231: if (line_count == 0) return false; it should return true, as it copied all the lines. https://codereview.chromium.org/424973004/diff/180001/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/424973004/diff/180001/src/profile-generator.h... src/profile-generator.h:69: bool Empty() const { return pc_offset_map_.empty(); } style: s/Empty/empty/ https://codereview.chromium.org/424973004/diff/180001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/180001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1079: "function %s() {\n" Does the test function need to be that complex? There seems to be no need to run loops in it. Isn't it?
I'm fine with the change once the couple of issues I pointed out in the code are addressed. Also it is unclear to me whether you are going to add column information to the hit count map or you expect it to be done by someone else, please clarify. https://codereview.chromium.org/424973004/diff/200001/src/cpu-profiler.cc File src/cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/200001/src/cpu-profiler.cc#new... src/cpu-profiler.cc:261: if (shared->script()->IsScript()) { It used to be a DCHECK(Script::cast(shared->script())), why we now check if it is a Script? https://codereview.chromium.org/424973004/diff/200001/src/cpu-profiler.cc#new... src/cpu-profiler.cc:287: rec->entry->set_bailout_reason( nit: no need to move these two lines https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.c... src/profile-generator.cc:652: src_line = pc_entry->GetSourceLine(pc_offset); nit: move assignment to src_line after the "if" below. https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.c... src/profile-generator.cc:688: if (src_line_not_found && *entry) { With this approach we may end up with top entry being pc_entry while src_line will be calculated for another entry which is wrong. We should also check here that current entry is the first one non-null in the path. https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.h... src/profile-generator.h:64: return v8::CpuProfileNode::kNoLineNumberInfo; It would be more accurate to return last line in this case. Otherwise we will attribute current sample to the last line only in case of perfect pc_offset match. https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1099: if (func->code()->is_optimized_code()) { You can use "%NeverOptimizeFunction(func)" or %OptimizeFunctionOnNextCall to make this more deterministic. https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1138: CHECK_EQ(false, line_info->Empty()); nit: CHECK(!line_info->Empty())
The yurys's comments were addressed. About adding column information to the hit count map - I or someone from Weiliang team can it as a separate patch. https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.c... src/profile-generator.cc:688: if (src_line_not_found && *entry) { On 2014/09/10 08:05:14, yurys wrote: > With this approach we may end up with top entry being pc_entry while src_line > will be calculated for another entry which is wrong. We should also check here > that current entry is the first one non-null in the path. pc_offset that used to calculate src_line is taken for the first non-null entry in the path only. Exactly this entry will represent a node in profile (when the path and src_line go to AddPathToCurrentProfiles). https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1099: if (func->code()->is_optimized_code()) { On 2014/09/10 08:05:14, yurys wrote: > You can use "%NeverOptimizeFunction(func)" or %OptimizeFunctionOnNextCall to > make this more deterministic. The test should check optimized code also. I suggest to stay as is because to test both optimized and non-optimized version of function.
On 2014/09/11 08:14:44, Denis Pravdin wrote: > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > File test/cctest/test-cpu-profiler.cc (right): > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > test/cctest/test-cpu-profiler.cc:1099: if (func->code()->is_optimized_code()) { > On 2014/09/10 08:05:14, yurys wrote: > > You can use "%NeverOptimizeFunction(func)" or %OptimizeFunctionOnNextCall to > > make this more deterministic. > > The test should check optimized code also. I suggest to stay as is because to > test both optimized and non-optimized version of function. In that case the test should always check both versions while current implementation will behave differently depending on --no-crankshaft flag. Do we have any bots that run tests with that flag? If no then we will always test only one of the two paths. Also if the test depends on some command line flag you can explicitly set it right in the test. https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.c... src/profile-generator.cc:688: if (src_line_not_found && *entry) { On 2014/09/11 08:14:44, Denis Pravdin wrote: > On 2014/09/10 08:05:14, yurys wrote: > > With this approach we may end up with top entry being pc_entry while src_line > > will be calculated for another entry which is wrong. We should also check here > > that current entry is the first one non-null in the path. > > pc_offset that used to calculate src_line is taken for the first non-null entry > in the path only. Exactly this entry will represent a node in profile (when the > path and src_line go to AddPathToCurrentProfiles). If src_line = pc_entry->GetSourceLine(pc_offset); (line 652) returns kNoLineNumberInfo you will end up calculating line number for the first non-null entry after it here. This will result in entries = [pc_entry, entry1, entry2,...NULL] and src_line calculated for entry1 or am I missing something?
On 2014/09/12 13:46:28, yurys wrote: > On 2014/09/11 08:14:44, Denis Pravdin wrote: > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > File test/cctest/test-cpu-profiler.cc (right): > > > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > test/cctest/test-cpu-profiler.cc:1099: if (func->code()->is_optimized_code()) > { > > On 2014/09/10 08:05:14, yurys wrote: > > > You can use "%NeverOptimizeFunction(func)" or %OptimizeFunctionOnNextCall to > > > make this more deterministic. > > > > The test should check optimized code also. I suggest to stay as is because to > > test both optimized and non-optimized version of function. > > In that case the test should always check both versions while current > implementation will behave differently depending on --no-crankshaft flag. Do we > have any bots that run tests with that flag? If no then we will always test only > one of the two paths. Also if the test depends on some command line flag you can > explicitly set it right in the test. According to run-tests.py, each test will run four times using below four variant flags seperately. VARIANT_FLAGS = { "default": [], "stress": ["--stress-opt", "--always-opt"], "turbofan": ["--turbo-filter=*", "--always-opt"], "nocrankshaft": ["--nocrankshaft"]} So we think the optimized/unoptimized code will be test here. > > https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.cc > File src/profile-generator.cc (right): > > https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.c... > src/profile-generator.cc:688: if (src_line_not_found && *entry) { > On 2014/09/11 08:14:44, Denis Pravdin wrote: > > On 2014/09/10 08:05:14, yurys wrote: > > > With this approach we may end up with top entry being pc_entry while > src_line > > > will be calculated for another entry which is wrong. We should also check > here > > > that current entry is the first one non-null in the path. > > > > pc_offset that used to calculate src_line is taken for the first non-null > entry > > in the path only. Exactly this entry will represent a node in profile (when > the > > path and src_line go to AddPathToCurrentProfiles). > > If src_line = pc_entry->GetSourceLine(pc_offset); (line 652) returns > kNoLineNumberInfo you will end up calculating line number for the first non-null > entry after it here. This will result in entries = [pc_entry, entry1, > entry2,...NULL] and src_line calculated for entry1 or am I missing something?
On 2014/09/15 01:50:16, Weiliang wrote: > On 2014/09/12 13:46:28, yurys wrote: > > On 2014/09/11 08:14:44, Denis Pravdin wrote: > > > > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > > File test/cctest/test-cpu-profiler.cc (right): > > > > > > > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > > test/cctest/test-cpu-profiler.cc:1099: if > (func->code()->is_optimized_code()) > > { > > > On 2014/09/10 08:05:14, yurys wrote: > > > > You can use "%NeverOptimizeFunction(func)" or %OptimizeFunctionOnNextCall > to > > > > make this more deterministic. > > > > > > The test should check optimized code also. I suggest to stay as is because > to > > > test both optimized and non-optimized version of function. > > > > In that case the test should always check both versions while current > > implementation will behave differently depending on --no-crankshaft flag. Do > we > > have any bots that run tests with that flag? If no then we will always test > only > > one of the two paths. Also if the test depends on some command line flag you > can > > explicitly set it right in the test. > > According to run-tests.py, each test will run four times using below four > variant flags seperately. > VARIANT_FLAGS = { > "default": [], > "stress": ["--stress-opt", "--always-opt"], > "turbofan": ["--turbo-filter=*", "--always-opt"], > "nocrankshaft": ["--nocrankshaft"]} > > So we think the optimized/unoptimized code will be test here. > OK, that makes sense.
On 2014/09/12 13:46:28, yurys wrote: > On 2014/09/11 08:14:44, Denis Pravdin wrote: > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > File test/cctest/test-cpu-profiler.cc (right): > > > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > test/cctest/test-cpu-profiler.cc:1099: if (func->code()->is_optimized_code()) > { > > On 2014/09/10 08:05:14, yurys wrote: > > > You can use "%NeverOptimizeFunction(func)" or %OptimizeFunctionOnNextCall to > > > make this more deterministic. > > > > The test should check optimized code also. I suggest to stay as is because to > > test both optimized and non-optimized version of function. > > In that case the test should always check both versions while current > implementation will behave differently depending on --no-crankshaft flag. Do we > have any bots that run tests with that flag? If no then we will always test only > one of the two paths. Also if the test depends on some command line flag you can > explicitly set it right in the test. > > https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.cc > File src/profile-generator.cc (right): > > https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.c... > src/profile-generator.cc:688: if (src_line_not_found && *entry) { > On 2014/09/11 08:14:44, Denis Pravdin wrote: > > On 2014/09/10 08:05:14, yurys wrote: > > > With this approach we may end up with top entry being pc_entry while > src_line > > > will be calculated for another entry which is wrong. We should also check > here > > > that current entry is the first one non-null in the path. > > > > pc_offset that used to calculate src_line is taken for the first non-null > entry > > in the path only. Exactly this entry will represent a node in profile (when > the > > path and src_line go to AddPathToCurrentProfiles). > > If src_line = pc_entry->GetSourceLine(pc_offset); (line 652) returns > kNoLineNumberInfo you will end up calculating line number for the first non-null > entry after it here. This will result in entries = [pc_entry, entry1, > entry2,...NULL] and src_line calculated for entry1 or am I missing something? Yes, you got it right - line number is calculated for the first non-null entry in the stack but not for sample frame. For example, it may happen when a sampled JS function does not have line number info (positions in reloc info). Is it possible situation for JS function? To fix we can use line_number() method of Code object if pc_entry does not have line number info. If pc_entry->line_number() returns kNoLineNumberInfo that src_line = 0. Will it work?
On 2014/09/15 06:25:38, yurys wrote: > On 2014/09/15 01:50:16, Weiliang wrote: > > On 2014/09/12 13:46:28, yurys wrote: > > > On 2014/09/11 08:14:44, Denis Pravdin wrote: > > > > > > > > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > > > File test/cctest/test-cpu-profiler.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > > > test/cctest/test-cpu-profiler.cc:1099: if > > (func->code()->is_optimized_code()) > > > { > > > > On 2014/09/10 08:05:14, yurys wrote: > > > > > You can use "%NeverOptimizeFunction(func)" or > %OptimizeFunctionOnNextCall > > to > > > > > make this more deterministic. > > > > > > > > The test should check optimized code also. I suggest to stay as is because > > to > > > > test both optimized and non-optimized version of function. > > > > > > In that case the test should always check both versions while current > > > implementation will behave differently depending on --no-crankshaft flag. Do > > we > > > have any bots that run tests with that flag? If no then we will always test > > only > > > one of the two paths. Also if the test depends on some command line flag you > > can > > > explicitly set it right in the test. > > > > According to run-tests.py, each test will run four times using below four > > variant flags seperately. > > VARIANT_FLAGS = { > > "default": [], > > "stress": ["--stress-opt", "--always-opt"], > > "turbofan": ["--turbo-filter=*", "--always-opt"], > > "nocrankshaft": ["--nocrankshaft"]} > > > > So we think the optimized/unoptimized code will be test here. > > > OK, that makes sense. Hi yurys, Please take a look at patch set 13 which tried to address your last comment. Thanks -Weiliang
On 2014/09/15 09:52:06, Denis Pravdin wrote: > On 2014/09/12 13:46:28, yurys wrote: > > On 2014/09/11 08:14:44, Denis Pravdin wrote: > > > > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > > File test/cctest/test-cpu-profiler.cc (right): > > > > > > > > > https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-pr... > > > test/cctest/test-cpu-profiler.cc:1099: if > (func->code()->is_optimized_code()) > > { > > > On 2014/09/10 08:05:14, yurys wrote: > > > > You can use "%NeverOptimizeFunction(func)" or %OptimizeFunctionOnNextCall > to > > > > make this more deterministic. > > > > > > The test should check optimized code also. I suggest to stay as is because > to > > > test both optimized and non-optimized version of function. > > > > In that case the test should always check both versions while current > > implementation will behave differently depending on --no-crankshaft flag. Do > we > > have any bots that run tests with that flag? If no then we will always test > only > > one of the two paths. Also if the test depends on some command line flag you > can > > explicitly set it right in the test. > > > > https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.cc > > File src/profile-generator.cc (right): > > > > > https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.c... > > src/profile-generator.cc:688: if (src_line_not_found && *entry) { > > On 2014/09/11 08:14:44, Denis Pravdin wrote: > > > On 2014/09/10 08:05:14, yurys wrote: > > > > With this approach we may end up with top entry being pc_entry while > > src_line > > > > will be calculated for another entry which is wrong. We should also check > > here > > > > that current entry is the first one non-null in the path. > > > > > > pc_offset that used to calculate src_line is taken for the first non-null > > entry > > > in the path only. Exactly this entry will represent a node in profile (when > > the > > > path and src_line go to AddPathToCurrentProfiles). > > > > If src_line = pc_entry->GetSourceLine(pc_offset); (line 652) returns > > kNoLineNumberInfo you will end up calculating line number for the first > non-null > > entry after it here. This will result in entries = [pc_entry, entry1, > > entry2,...NULL] and src_line calculated for entry1 or am I missing something? > > Yes, you got it right - line number is calculated for the first non-null entry > in the stack but not for sample frame. For example, it may happen when a sampled > JS function does not have line number info (positions in reloc info). Is it > possible situation for JS function? > I'm not sure if, but I think it might happen for some functions loaded from the snapshot, also functions with hand-written code will not have such info I guess. > To fix we can use line_number() method of Code object if pc_entry does not have > line number info. If pc_entry->line_number() returns kNoLineNumberInfo that > src_line = 0. > Will it work? Yes, I think that should be fine to attribute all hits to the first line for the code objects that don't have line info.
lgtm
On 2014/09/16 10:10:39, yurys wrote:
> lgtm
Hi Yurys
Please see again the patch set 15.
I have rebased it and fixed the failure I talked with you when turbo fan is
turned on. The root cause of the failure is turbo fan enables --turbo_types flag
by default recently and is able to emit better JIT code with less IC CALL(For
example, no IC call now is emitted by TF for the example function("func") of
TickLines test). And turbo fan only emits source position info if
FLAG_turbo_source_positions is on or the node is a Call. That is why there is no
line info after rebasing for turbo fan.
Thanks
-Weiliang
I will take care of landing the cl.
On 2014/09/17 03:14:12, Weiliang wrote:
> On 2014/09/16 10:10:39, yurys wrote:
> > lgtm
>
> Hi Yurys
>
> Please see again the patch set 15.
>
> I have rebased it and fixed the failure I talked with you when turbo fan is
> turned on. The root cause of the failure is turbo fan enables --turbo_types
flag
> by default recently and is able to emit better JIT code with less IC CALL(For
> example, no IC call now is emitted by TF for the example function("func") of
> TickLines test). And turbo fan only emits source position info if
> FLAG_turbo_source_positions is on or the node is a Call. That is why there is
no
> line info after rebasing for turbo fan.
>
Is it going to be enabled by default or will we miss source line positions with
turbo fan?
On 2014/09/17 10:04:11, yurys wrote:
> I will take care of landing the cl.
>
> On 2014/09/17 03:14:12, Weiliang wrote:
> > On 2014/09/16 10:10:39, yurys wrote:
> > > lgtm
> >
> > Hi Yurys
> >
> > Please see again the patch set 15.
> >
> > I have rebased it and fixed the failure I talked with you when turbo fan is
> > turned on. The root cause of the failure is turbo fan enables --turbo_types
> flag
> > by default recently and is able to emit better JIT code with less IC
CALL(For
> > example, no IC call now is emitted by TF for the example function("func") of
> > TickLines test). And turbo fan only emits source position info if
> > FLAG_turbo_source_positions is on or the node is a Call. That is why there
is
> no
> > line info after rebasing for turbo fan.
> >
> Is it going to be enabled by default or will we miss source line positions
with
> turbo fan?
Thanks.
I am not sure whether it (FLAG_turbo_source_positions) will be enabled by
default. @danno, @titzer may know.
Thanks
-Weiliang
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/424973004/320001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2014/09/17 13:48:21, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer or > a provisional committer, _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Kindly ping :)
On 2014/09/18 13:59:24, Weiliang wrote: > On 2014/09/17 13:48:21, I haz the power (commit-bot) wrote: > > No LGTM from a valid reviewer yet. Only full committers are accepted. > > Even if an LGTM may have been provided, it was from a non-committer or > > a provisional committer, _not_ a full super star committer. > > See http://www.chromium.org/getting-involved/become-a-committer > > Note that this has nothing to do with OWNERS files. > > Kindly ping :) Looks like a few tests are failing consistently on v8_linux_nosnap_dbg, can you take a look if it is related to the patch?
Patchset #16 (id:340001) has been deleted
On 2014/09/18 16:09:30, yurys wrote: > On 2014/09/18 13:59:24, Weiliang wrote: > > On 2014/09/17 13:48:21, I haz the power (commit-bot) wrote: > > > No LGTM from a valid reviewer yet. Only full committers are accepted. > > > Even if an LGTM may have been provided, it was from a non-committer or > > > a provisional committer, _not_ a full super star committer. > > > See http://www.chromium.org/getting-involved/become-a-committer > > > Note that this has nothing to do with OWNERS files. > > > > Kindly ping :) > > Looks like a few tests are failing consistently on v8_linux_nosnap_dbg, can you > take a look if it is related to the patch? Hi Yurys, Denis gave the patch set 16 to fix the v8_linux_nosnap_dbg failures. Could you please have a look? The idea of this fix is to avoid access Code object through a any given Address The Address may be fake and is unable to map to a Code object, just like what cctest/test-profile-generator/RecordTickSample does. The old buggy patch lowers the access bar and leads to the failures. Thanks -Weiliang
On 2014/09/26 14:03:10, Weiliang wrote: > On 2014/09/18 16:09:30, yurys wrote: > > On 2014/09/18 13:59:24, Weiliang wrote: > > > On 2014/09/17 13:48:21, I haz the power (commit-bot) wrote: > > > > No LGTM from a valid reviewer yet. Only full committers are accepted. > > > > Even if an LGTM may have been provided, it was from a non-committer or > > > > a provisional committer, _not_ a full super star committer. > > > > See http://www.chromium.org/getting-involved/become-a-committer > > > > Note that this has nothing to do with OWNERS files. > > > > > > Kindly ping :) > > > > Looks like a few tests are failing consistently on v8_linux_nosnap_dbg, can > you > > take a look if it is related to the patch? > > Hi Yurys, > Denis gave the patch set 16 to fix the v8_linux_nosnap_dbg failures. Could you > please have a look? > > The idea of this fix is to avoid access Code object through a any given Address > The Address may be fake and is unable to map to a Code object, just like what > cctest/test-profile-generator/RecordTickSample does. The old buggy patch lowers > the access bar and leads to the failures. > > Thanks > -Weiliang Kindly ping.
On 2014/10/02 07:26:28, Denis Pravdin wrote: > On 2014/09/26 14:03:10, Weiliang wrote: > > On 2014/09/18 16:09:30, yurys wrote: > > > On 2014/09/18 13:59:24, Weiliang wrote: > > > > On 2014/09/17 13:48:21, I haz the power (commit-bot) wrote: > > > > > No LGTM from a valid reviewer yet. Only full committers are accepted. > > > > > Even if an LGTM may have been provided, it was from a non-committer or > > > > > a provisional committer, _not_ a full super star committer. > > > > > See http://www.chromium.org/getting-involved/become-a-committer > > > > > Note that this has nothing to do with OWNERS files. > > > > > > > > Kindly ping :) > > > > > > Looks like a few tests are failing consistently on v8_linux_nosnap_dbg, can > > you > > > take a look if it is related to the patch? > > > > Hi Yurys, > > Denis gave the patch set 16 to fix the v8_linux_nosnap_dbg failures. Could you > > please have a look? > > > > The idea of this fix is to avoid access Code object through a any given > Address > > The Address may be fake and is unable to map to a Code object, just like what > > cctest/test-profile-generator/RecordTickSample does. The old buggy patch > lowers > > the access bar and leads to the failures. > > > > Thanks > > -Weiliang > > Kindly ping. Oops, I thought that it was landed. Sorry about that. Patch set #16 - lgtm, I will land it.
I applied the patch locally and getting the following error when building x64.debug: $ make x64.debug.check -j300 make[1]: Entering directory `/sources/chromium/src/v8/out' LINK(target) /sources/chromium/src/v8/out/x64.debug/mksnapshot /sources/chromium/src/v8/out/x64.debug/obj.target/v8_base/src/cpu-profiler.o: In function `v8::internal::JITLineInfoTable::GetSourceLineNumber(int) const': /sources/chromium/src/v8/out/.././src/profile-generator.h:67: undefined reference to `v8::CpuProfileNode::kNoLineNumberInfo' collect2: ld returned 1 exit status make[1]: *** [/sources/chromium/src/v8/out/x64.debug/mksnapshot] Error 1 make[1]: Leaving directory `/sources/chromium/src/v8/out' make: *** [x64.debug] Error 2 Could you please fix that? Also it might make sense to move implementation of JITLineInfoTable methods into profile-generator.cc for better readability. But it is up to you.
On 2014/10/02 07:54:52, yurys wrote:
> I applied the patch locally and getting the following error when building
> x64.debug:
>
> $ make x64.debug.check -j300
> make[1]: Entering directory `/sources/chromium/src/v8/out'
> LINK(target) /sources/chromium/src/v8/out/x64.debug/mksnapshot
> /sources/chromium/src/v8/out/x64.debug/obj.target/v8_base/src/cpu-profiler.o:
In
> function `v8::internal::JITLineInfoTable::GetSourceLineNumber(int) const':
> /sources/chromium/src/v8/out/.././src/profile-generator.h:67: undefined
> reference to `v8::CpuProfileNode::kNoLineNumberInfo'
> collect2: ld returned 1 exit status
> make[1]: *** [/sources/chromium/src/v8/out/x64.debug/mksnapshot] Error 1
> make[1]: Leaving directory `/sources/chromium/src/v8/out'
> make: *** [x64.debug] Error 2
>
> Could you please fix that?
>
> Also it might make sense to move implementation of JITLineInfoTable methods
into
> profile-generator.cc for better readability. But it is up to you.
Yeah, I saw this linker error on Linux debug configuration only after I applied
my patch. But it seems that the problem is there even before my patch.
The include/v8-profiler.h file includes a constant initializer for
kNoLineNumberInfo. I know that class static const integral member is not
required to be defined out of class. But it's better to define
CpuProfileNode::kNoLineNumberInfo somewhere.
I fixed it locally, just for testing.
src/cpu_profiler.cc:
namespace v8 {
const int CpuProfileNode::kNoLineNumberInfo;
}
I can't permissions to upload the patch (that's a reason I am asking Weiliang to
help me with the patch) for review. Weiliang is OOO to Oct 7 due to PRC national
Holiday.
Can you fix the linker error and then apply my patch or you prefer to wait for
one more patch?
Thanks,
Denis
The linker error looks like a bug in gcc as the following change fixes it: - return pc_offset_map_.empty() ? (v8::CpuProfileNode::kNoLineNumberInfo) : (--pc_offset_map_.end())->second; + if (pc_offset_map_.empty()) + return v8::CpuProfileNode::kNoLineNumberInfo; + return (--pc_offset_map_.end())->second;
On 2014/10/02 08:26:38, Denis Pravdin wrote:
> On 2014/10/02 07:54:52, yurys wrote:
> > I applied the patch locally and getting the following error when building
> > x64.debug:
> >
> > $ make x64.debug.check -j300
> > make[1]: Entering directory `/sources/chromium/src/v8/out'
> > LINK(target) /sources/chromium/src/v8/out/x64.debug/mksnapshot
> >
/sources/chromium/src/v8/out/x64.debug/obj.target/v8_base/src/cpu-profiler.o:
> In
> > function `v8::internal::JITLineInfoTable::GetSourceLineNumber(int) const':
> > /sources/chromium/src/v8/out/.././src/profile-generator.h:67: undefined
> > reference to `v8::CpuProfileNode::kNoLineNumberInfo'
> > collect2: ld returned 1 exit status
> > make[1]: *** [/sources/chromium/src/v8/out/x64.debug/mksnapshot] Error 1
> > make[1]: Leaving directory `/sources/chromium/src/v8/out'
> > make: *** [x64.debug] Error 2
> >
> > Could you please fix that?
> >
> > Also it might make sense to move implementation of JITLineInfoTable methods
> into
> > profile-generator.cc for better readability. But it is up to you.
>
>
> Yeah, I saw this linker error on Linux debug configuration only after I
applied
> my patch. But it seems that the problem is there even before my patch.
> The include/v8-profiler.h file includes a constant initializer for
> kNoLineNumberInfo. I know that class static const integral member is not
> required to be defined out of class. But it's better to define
> CpuProfileNode::kNoLineNumberInfo somewhere.
>
> I fixed it locally, just for testing.
> src/cpu_profiler.cc:
> namespace v8 {
> const int CpuProfileNode::kNoLineNumberInfo;
> }
>
> I can't permissions to upload the patch (that's a reason I am asking Weiliang
to
> help me with the patch) for review. Weiliang is OOO to Oct 7 due to PRC
national
> Holiday.
>
> Can you fix the linker error and then apply my patch or you prefer to wait for
> one more patch?
>
Done. Uploaded your patch and a fix on top of it as
https://codereview.chromium.org/616963005/
> Thanks,
> Denis
On 2014/10/02 08:41:26, yurys wrote:
> On 2014/10/02 08:26:38, Denis Pravdin wrote:
> > On 2014/10/02 07:54:52, yurys wrote:
> > > I applied the patch locally and getting the following error when building
> > > x64.debug:
> > >
> > > $ make x64.debug.check -j300
> > > make[1]: Entering directory `/sources/chromium/src/v8/out'
> > > LINK(target) /sources/chromium/src/v8/out/x64.debug/mksnapshot
> > >
> /sources/chromium/src/v8/out/x64.debug/obj.target/v8_base/src/cpu-profiler.o:
> > In
> > > function `v8::internal::JITLineInfoTable::GetSourceLineNumber(int) const':
> > > /sources/chromium/src/v8/out/.././src/profile-generator.h:67: undefined
> > > reference to `v8::CpuProfileNode::kNoLineNumberInfo'
> > > collect2: ld returned 1 exit status
> > > make[1]: *** [/sources/chromium/src/v8/out/x64.debug/mksnapshot] Error 1
> > > make[1]: Leaving directory `/sources/chromium/src/v8/out'
> > > make: *** [x64.debug] Error 2
> > >
> > > Could you please fix that?
> > >
> > > Also it might make sense to move implementation of JITLineInfoTable
methods
> > into
> > > profile-generator.cc for better readability. But it is up to you.
> >
> >
> > Yeah, I saw this linker error on Linux debug configuration only after I
> applied
> > my patch. But it seems that the problem is there even before my patch.
> > The include/v8-profiler.h file includes a constant initializer for
> > kNoLineNumberInfo. I know that class static const integral member is not
> > required to be defined out of class. But it's better to define
> > CpuProfileNode::kNoLineNumberInfo somewhere.
> >
> > I fixed it locally, just for testing.
> > src/cpu_profiler.cc:
> > namespace v8 {
> > const int CpuProfileNode::kNoLineNumberInfo;
> > }
> >
> > I can't permissions to upload the patch (that's a reason I am asking
Weiliang
> to
> > help me with the patch) for review. Weiliang is OOO to Oct 7 due to PRC
> national
> > Holiday.
> >
> > Can you fix the linker error and then apply my patch or you prefer to wait
for
> > one more patch?
> >
> Done. Uploaded your patch and a fix on top of it as
> https://codereview.chromium.org/616963005/
>
>
> > Thanks,
> > Denis
Thank you!
On 2014/10/02 08:41:26, yurys wrote:
> On 2014/10/02 08:26:38, Denis Pravdin wrote:
> > On 2014/10/02 07:54:52, yurys wrote:
> > > I applied the patch locally and getting the following error when building
> > > x64.debug:
> > >
> > > $ make x64.debug.check -j300
> > > make[1]: Entering directory `/sources/chromium/src/v8/out'
> > > LINK(target) /sources/chromium/src/v8/out/x64.debug/mksnapshot
> > >
> /sources/chromium/src/v8/out/x64.debug/obj.target/v8_base/src/cpu-profiler.o:
> > In
> > > function `v8::internal::JITLineInfoTable::GetSourceLineNumber(int) const':
> > > /sources/chromium/src/v8/out/.././src/profile-generator.h:67: undefined
> > > reference to `v8::CpuProfileNode::kNoLineNumberInfo'
> > > collect2: ld returned 1 exit status
> > > make[1]: *** [/sources/chromium/src/v8/out/x64.debug/mksnapshot] Error 1
> > > make[1]: Leaving directory `/sources/chromium/src/v8/out'
> > > make: *** [x64.debug] Error 2
> > >
> > > Could you please fix that?
> > >
> > > Also it might make sense to move implementation of JITLineInfoTable
methods
> > into
> > > profile-generator.cc for better readability. But it is up to you.
> >
> >
> > Yeah, I saw this linker error on Linux debug configuration only after I
> applied
> > my patch. But it seems that the problem is there even before my patch.
> > The include/v8-profiler.h file includes a constant initializer for
> > kNoLineNumberInfo. I know that class static const integral member is not
> > required to be defined out of class. But it's better to define
> > CpuProfileNode::kNoLineNumberInfo somewhere.
> >
> > I fixed it locally, just for testing.
> > src/cpu_profiler.cc:
> > namespace v8 {
> > const int CpuProfileNode::kNoLineNumberInfo;
> > }
> >
> > I can't permissions to upload the patch (that's a reason I am asking
Weiliang
> to
> > help me with the patch) for review. Weiliang is OOO to Oct 7 due to PRC
> national
> > Holiday.
> >
> > Can you fix the linker error and then apply my patch or you prefer to wait
for
> > one more patch?
> >
> Done. Uploaded your patch and a fix on top of it as
> https://codereview.chromium.org/616963005/
>
>
> > Thanks,
> > Denis
The change was committed as https://code.google.com/p/v8/source/detail?r=24389,
closing this issue.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
