RFR (xs) 8145940: TempNewSymbol should have correct copy and assignment functions

Lois Foltan lois.foltan at oracle.com
Tue Jan 12 01:27:07 UTC 2016


On 1/11/2016 7:51 PM, Lois Foltan wrote:
>
> On 1/11/2016 6:45 PM, Kim Barrett wrote:
>> On Jan 7, 2016, at 3:02 PM, Coleen 
>> Phillimore<coleen.phillimore at oracle.com>  wrote:
>>> Summary: Add clear() to the assignment operator and add copy 
>>> constructor.
>>>
>>> Ran all jtreg, colocated and non-colocated tests.   RunThese -jck 
>>> with PrintSymbolTableSizeHistogram statistics:
>>>
>>>   Percent removed          1.35
>>>   Reference counts          194583
>>>
>>> clean:
>>>
>>>   Percent removed          1.53
>>>   Reference counts          194245
>>>
>>> Without a reference counting copy constructor, we could remove a 
>>> TempNewSymbol's Symbol if a GC happens.  Consider:
>>>
>>> TempNewSymbol ts = SymbolTable::new_symbol("abc");
>>> // Hit GC
>>>
>>> The ref count for "abc" is 1 when created by new_symbol, and the 
>>> destructor could run after the copy into ts, decrementing the 
>>> reference count to 0 again.  GC could unlink that symbol from the 
>>> symbol table.  Fortunately, we haven't seen this bug.
>>>
>>> open webrev athttp://cr.openjdk.java.net/~coleenp/8145940/
>>> bug linkhttps://bugs.openjdk.java.net/browse/JDK-8145940
>>>
>>> Thanks,
>>> Coleen
>> For the assignment operator, I think better is:
>>
>> void operator=(TempNewSymbol s) {
>>    Symbol* tmp = s._temp;
>>    s._temp = _temp;
>>    _temp = tmp;
>> }
>>
>> This is the well-known copy and swap idiom, with a by-hand "swap".
>
> Kim,
> I'm a bit confused, why would the assignment operator want to swap? I 
> don't think s._temp should be assigned _temp's value.

After some more thought on this, I would like to add I think you are 
doing the assignment into s._temp of the former value of _temp to get 
the destructor to run on the temporary that is created for the pass by 
value parameter s.  And thus if the destructor is run, the refcount is 
decremented of _temp's former value.  I find this way too subtle, and 
would prefer to actually see the assignment operator do the clear().
Lois

>
>> http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom 
>>
>>
>> In the copy constructor, as a matter of style I would initialize _temp
>> in the initialer list, rather than by assignment in the body.
>>
>> I think the updated description of TempNewSymbol isn't really right.
>> Because the conversion constructor doesn't increment the reference
>> count, it must *not* be used to capture some arbitrary reference to a
>> Symbol*. Only a new symbol or one obtained from one of the lookup
>> functions really should be managed by this class.
>
> I agree with this point, can we improve the comment ahead of 
> TempNewSymbol's class definition as part of this webrev?
> Thanks,
> Lois
>
>> Regarding copy elision, there's a good discussion here:
>>
>> http://en.cppreference.com/w/cpp/language/copy_elision
>>
>



More information about the hotspot-runtime-dev mailing list