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

[Vladimir Kozlov]

On 6/2/13 11:16 PM, Albert Noll wrote:
> HI Vladimir,
>
> thank you for your review. I made the suggested changes:
> http://cr.openjdk.java.net/~anoll/8013329/webrev.03/ <http://cr.openjdk.java.net/%7Eanoll/8013329/webrev.03/>

Looks.

> Is another round of reviews required?

No. Please, prepare patch.

Thanks,
Vladimir

>
> Best,
> Albert
>
> On 31.05.2013 18:03, Vladimir Kozlov wrote:
>> 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
>