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

[Albert Noll]

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/>

Is another round of reviews required?

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20130603/6acce911/attachment.html