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

[Coleen Phillimore]

On 1/13/16 6:29 PM, John Rose wrote:
> Count me in as one who is very glad to see TempNewSymbol
> getting regularized.  You can also count me as a reviewer.
>
> Like Kim and Markus, I dislike the fact that TempNewSymbol
> can decrease a refcount that it did not increase in the first place,
> because new_symbol or lookup pre-incremented the refcount.
> (FWIW, this has always bothered me about TempNewSymbol.)
> This is the sort of convention that works best when the type
> system enforces it.  Quick question:  Exactly how many of
> the Symbol::* API points which yield Symbol* values include
> the pre-increment?  If the answer is not "all of them" or "none
> of them", we have a manually-enforced distinction which may
> turn out to be fragile.

It's all but two.   Yes, it can be fragile.  So the reason for having 
TempNewSymbol in the first place was to track symbols coming into the 
JVM via char*, vs. via the constant pool.  At the time it was 90% the 
constant pool so we didn't want refcounting for every symbol operation, 
ala smart pointers.  It was a bigger not nice change at the time, and 
tons of refcounting.  We also really want to manipulate symbols as 
Symbol* rather than some other name but we've debated this lots of times.

>
> Markus, is there a tracking bug yet for addressing this?
> There should be.
>
> The "copy and swap idiom" is nice, and a new C++ trick to me.
> It is a clever way of cleanly encapsulating keeping the lifecycle
> invariants in the ctor/dtor pairs.  (Except for the case of TNS(S*)
> which Kim, Markus, and I are uncomfortable about.)
>
> Here's a thought, FWIW, about an incremental way forward on
> the pre-increment problem.  It might be useful to split the "magic"
> functions:
>
>   TempNewSymbol lookup(int index, const char* name, int len, unsigned 
> int hash);
>   TempNewSymbol new_symbol(const char* name, TRAPS) { … }
>   Symbol* lookup_increment_refcount(int index, const char* name, int 
> len, unsigned int hash);
>   Symbol* new_symbol_increment_refcount(const char* name, TRAPS) { … }
>
> There would be an initial wave of renaming (adding _increment_refcount),
> followed (perhaps) by gradual removal of the _increment_refcount versions
> in favor of more consistent use of TempNewSymbol.  Arrays are tricky, 
> alas,
> so maybe the endpoint is still messy, but elsewhere we work with growable
> arrays of type Handle, so maybe there is a way forward.

Having a safer API for creating TempNewSymbol with char* would be nice 
to have.

Coleen
>
> — John
>
>
> On Jan 13, 2016, at 2:35 PM, Kim Barrett <kim.barrett at oracle.com 
> <mailto:kim.barrett at oracle.com>> wrote:
>>
>> I also like Markus's suggestion of SymbolTable symbol access returning
>> TempNewSymbol (perhaps under a better name!), but that is likely a
>> bigger change; I think this is still an improvement.
>>
>> Looks good, other than those minor things above.  Unless one of these
>> leads to a substantive change, I don't need another webrev.
>