Thanks for the new webrev and explanations. ciReplay.hpp 59 // Replay data file replay_pid%p.log is also created when VM crushes
when VM crashes That looks good to me. Roland. On Jan 8, 2014, at 5:02 AM, Vladimir Kozlov <vladimir.koz...@oracle.com> wrote: > Thank you, Roland > > On 1/7/14 2:39 AM, Roland Westrelin wrote: >>> http://cr.openjdk.java.net/~kvn/8028468/webrev/ > > New webrev: > > http://cr.openjdk.java.net/~kvn/8028468/webrev.01/ > >> >> Should the agent/doc/cireplay.html be updated? Is there another doc on how >> to use the replay support somewhere (wiki)? > > cireplay.html describes how to use SA to extract compilation replay data from > core files. Nothing changed there with my changes. > I don't think we have any wiki for compilation replay. > > I added all replay command descriptions and examples into ciReplay.hpp. > >> >> ciReplay.cpp >> typos: >> // Use pointer because we may need to retirn inline records >> >> // Replay Inlinig >> >> vmError.cpp >> typo: >> // Do not overwite file during replay > > Typos are fixed. > >> >> compile.hpp >> Shouldn’t _replay_inline_data be private with an accessor? >> >> 1183 // Dump inlining replay data to the stream. >> 1184 void dump_inline_data(outputStream* out); >> 1185 void* _replay_inline_data; > > Done. > >> >> ciReplay.cpp >> Why was this changed? Why was it there in the first place? >> 1052 // Make sure we don't run with background compilation >> 1053 // BackgroundCompilation = false; > > I forgot to uncomment it. It is done only when compilation (not inlining) is > replayed. It is special case. Such replaying is done by putting compilation > task on compile queue immediately after VM initialization. So we should wait > (-Xbatch, -XX:-BackgroundCompilation) until it is finished because we don't > need to execute java code (there is no program to execute). And after > compilation we exit VM: > > void ciReplay::replay(TRAPS) { > int exit_code = replay_impl(THREAD); > > Threads::destroy_vm(); > > vm_exit(exit_code); > } > >> >> Existing fields in class CompileReplay that don’t start with _ make the code >> confusing: >> >> char* bufptr; >> char* buffer; >> int buffer_length; >> int buffer_end; >> int line_no; >> >> etc. Maybe it would be worthwhile to fix that as part of this change? > > I added '_' to all fields in this file. > >> >> Can buffer be a growableArray? If not factor this code and other similar >> code in a method? >> 437 if (pos + 1 >= buffer_length) { >> 438 int newl = buffer_length * 2; >> 439 char* newb = NEW_RESOURCE_ARRAY(char, newl); >> 440 memcpy(newb, buffer, pos); >> 441 buffer = newb; >> 442 buffer_length = newl; >> 443 } >> >> Same for: >> 445 // null terminate it, reset the pointer and process the line >> 446 buffer[pos] = '\0'; >> 447 buffer_end = pos++; >> 448 bufptr = buffer; > > Done. That code is moved to new method get_line(). > >> >> Shouldn’t the fields below be private? >> 422 ciKlass* _iklass; >> 423 Method* _imethod; >> 424 int _entry_bci; >> 425 int _comp_level; > > Done. > >> >> I think >> ciInlineRecord* find_ciInlineRecord(Method* method, int bci, int >> inline_depth) >> should be implemented by calling: >> static ciInlineRecord* find_ciInlineRecord(GrowableArray<ciInlineRecord*>* >> records, Method* method, int bci, int inline_depth) >> to avoid duplication of code. > > Done. > > I also missed _ci_inline_records field initialization so I added it to > CompileReplay constructor. > > Thanks, > Vladimir > >> >> Roland. >>
