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

[Coleen Phillimore]

Kim and Lois,

Thanks for looking at this.   This is my latest webrev with the 
ExecuteInternalVMTest in it:

http://cr.openjdk.java.net/~coleenp/8145940.02/webrev/index.html

See comments below.

On 1/11/16 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 at http://cr.openjdk.java.net/~coleenp/8145940/
>> bug link https://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".
>
> http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom

Very tricky.  Lois and I figured out the trick.  It doesn't elide any 
Atomic::* reference counting though, which is what is worrysome for 
performance, just hides it in the destructor of 's'.  I don't want to 
change this.
> In the copy constructor, as a matter of style I would initialize _temp
> in the initialer list, rather than by assignment in the body.

Fixed.

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

Yes, this is a subtlety of the design and one that I can't really fix 
right now.  I thought of changing TempNewSymbol to convert from const 
char * but there are cases where we don't want to add a symbol.

There is a comment above TempNewSymbol.  I don't know what to add to 
it.   And Markus's comments are really good.

Thanks,
Coleen
> Regarding copy elision, there's a good discussion here:
>
> http://en.cppreference.com/w/cpp/language/copy_elision
>