RFR(S): 8013329: [parfait] File leak in hotspot/src/share/vm/compiler/compileBroker.cpp

Vladimir Kozlov vladimir.kozlov at oracle.com
Fri May 31 09:03:49 PDT 2013


Changes are good. I have only small comments:


Add {} for "if (LogCompilation && Verbose)".


instead of:

+  CompileLog::_first = NULL;


+  _first = NULL;


On 5/30/13 10:26 PM, Albert Noll wrote:
> Hi Roland,
> thanks again for looking at the code.
> On 28.05.2013 15:21, Roland Westrelin wrote:
>> Hi Albert,
>>>> FREE_C_HEAP_ARRAY is for arrays allocated with NEW_C_HEAP_ARRAY. You should use delete here, I think (even though FREE_C_HEAP_ARRAY and delete end up doing the same).
>>> The char array is allocated with NEW_HEAP_ARRAY in compileBroker.cpp. In the new version, I moved
>>> the allocation of the array into the constructor of CompileLog. I think that this makes the code better readable. Do you agree?
>> You're right. I misread the code.
>> It's better if it's allocated from the constructor and freed from the destructor, indeed.
>> Your change to CompileBroker::init_compiler_thread_log() is not entirely correct, I believe: if several temp dirs are tried, and the first ones fail, "Cannot open log file:" will be reported for the failed attempt even if the log file is successfully open in the end.
> Thanks for pointing that out. The new version prints the warning only if no file can be opened.
>>>> In CompileLog::finish_log_on_error, you delete log but use it afterwards log = log->_next.
>>> Ouch :-( Thanks for this one. I think the current version should be fine.
>> I think so as well.
>> Roland.
> When executing the current path with "fastdebug", the following warning appears:
> Warning:  TraceDependencies results may be inflated by VerifyDependencies
> I am not sure what that means. I tested with -XX:+LogCompilation and it seems to
> work fine.
> webrev: http://cr.openjdk.java.net/~anoll/8013329/webrev.02/ <http://cr.openjdk.java.net/%7Eanoll/8013329/webrev.02/>
> Thanks again,
> Albert

More information about the hotspot-compiler-dev mailing list