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

Lois Foltan lois.foltan at oracle.com
Tue Jan 12 00:51:30 UTC 2016

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".

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.

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

> 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