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

[Vladimir Kozlov]

Albert,

Changes are good. I have only small comments:

compileBroker.cpp:

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

compileLog.cpp:

instead of:

+  CompileLog::_first = NULL;

simple:

+  _first = NULL;

Thanks,
Vladimir


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