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

[Markus Gronlund]

Hi John,

Thanks for the input.

"Markus, is there a tracking bug yet for addressing this?
There should be."

https://bugs.openjdk.java.net/browse/JDK-8147083

Thanks again
Markus

-----Original Message-----
From: John Rose 
Sent: den 14 januari 2016 00:30
To: Kim Barrett; Coleen Phillimore
Cc: hotspot-runtime-dev
Subject: Re: RFR (xs) 8145940: TempNewSymbol should have correct copy and assignment functions

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.

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.

- John


On Jan 13, 2016, at 2:35 PM, Kim Barrett <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.