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

[Markus Gronlund]

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.

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.


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 link https://bugs.openjdk.java.net/browse/JDK-8145940

Please let me know what you think.

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 at http://cr.openjdk.java.net/~coleenp/8145940/
bug link https://bugs.openjdk.java.net/browse/JDK-8145940

Thanks,
Coleen