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

[Coleen Phillimore]

On 1/9/16 12:05 PM, Markus Gronlund wrote:
> Hi Coleen,
>
> Thanks for bringing this issue to attention, I actually stumbled into issues with this code in some contexts that I worked on last week.

Hi Markus,

Thank you for reading this and your extensive comments.

> I agree that it needs to be reworked, however I think my reasons differ a bit from the reason you put forth, which I believe is not a real problem.
>
> If I understand correctly, you are worried about the context which you describe with:
>
> "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."
>
> Actually I think this is not really a problem - this is because this will not call the copy constructor. Instead, it will call the type conversion constructor:
>
> TempNewSymbol(Symbol* s) : _temp(s) {}
>
> Since this constructor does not increment or decrement the _refcount, you are ok from a GC perspective.

The right hand side of the equal sign will call the conversion operator 
to create a TempNewSymbol and then call the copy constructor to get to 
the left hand side.

Most (all?) compilers elide this copy constructor away, but they don't 
have to.  If you make the copy constructor private, the compilers will 
give you a compilation error.  So logically, it does call the copy 
constructor, but we don't have a bug because it doesn't in real life.

>
> However, there are multiple other ways that TempNewSymbol can mess up the reference counting.
>
> I have summarized my analysis of both existing behavior as well as the suggested fix here:
>
> 814594:
>
> Problem description:http://cr.openjdk.java.net/~mgronlun/8145940/8145940_TempNewSymbol_analysis.txt  
> Webrev suggesting new impl:http://cr.openjdk.java.net/~mgronlun/8145940/webrev01/  
> bug linkhttps://bugs.openjdk.java.net/browse/JDK-8145940
>
> Please let me know what you think.
I think your change is essentially the same except for the self 
assignment and the really good comments.

+ if (this != &rhs && _temp != rhs._temp) {

This seems like a good optimization.  The original intent with 
TempNewSymbol and the Symbol* change was to minimize refcounting as much 
as possible.

I had comments to your analysis but I'll summarize them:


 > Since refcounts are dropped in TempNewSymbol's destructor, this means 
_refcount will be
 > decremented twice, since the second destructor to run will bring 
refcount to -1 (fortunately we
 > have an existing Symbol* _refcount underflow assertion in place to 
guard for this).

Right, but we don't want to hit that assertion!


 > Only concern is why //clear();  //FIXME was not implemented in the 
first place? Maybe this was
 > done because it is unclear what side-effects this fix might be 
causing if existing logic depend on this
 > leaking behavior? However, the right thing to do is to find those 
sites and correct them.

Yes, this was added as a comment but we were unsure if there would be a 
problem without additional
testing.

 > In addition, the clear() member function does not need to NULL out 
the current reference, hence:

Agreed.

So I want you to check in your code instead (reversing the code review) 
but I want to add an ExecuteInternalVMTest for it.

Coleen
> Best regards
> Markus
>
>
> -----Original Message-----
> From: Coleen Phillimore
> Sent: den 7 januari 2016 21:03
> To: hotspot-runtime-dev; Kim Barrett
> Subject: RFR (xs) 8145940: TempNewSymbol should have correct copy and assignment functions
>
> 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