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

[Markus Gronlund]

Hi Coleen,

 

Thanks for the clarification. See comments inline.

 

Best

Markus

 

From: Coleen Phillimore 
Sent: den 11 januari 2016 19:40
To: Markus Gronlund
Cc: hotspot-runtime-dev; Kim Barrett
Subject: Re: RFR (xs) 8145940: TempNewSymbol should have correct copy and assignment functions

 

 

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.

 
[MG] Interesting. I’m biased towards the MSFT CL compiler, and it has no problem with me disabling the copy constructor as well.... I guess this is as you say an area where compilers can do what they want…
 
What compilers would not compile because of a missing copy constructor here, do you know? 
 
And those that do, can they run without a copy constructor if instead it is being handed:
 
TempNewSymbol sym(SymbolTable::newsymbol(“abc”) – that is, it’s the “equal” sign that directs towards the copy constructor?
 
 
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: HYPERLINK "http://cr.openjdk.java.net/%7Emgronlun/8145940/8145940_TempNewSymbol_analysis.txt"http://cr.openjdk.java.net/~mgronlun/8145940/8145940_TempNewSymbol_analysis.txt 
Webrev suggesting new impl: HYPERLINK "http://cr.openjdk.java.net/%7Emgronlun/8145940/webrev01/"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.

I think your change is essentially the same except for the self assignment and the really good comments.



[MG] Agree, the reason for the extensive stuff is that I gathered all this info last week when debugging another issue – I also prepared a patch for this – when I saw your RFR on the exact same issue I just thought I would send it your way J

 

+    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!



Absolutely agree.

 



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

 

Understand now.



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



I have no problem with you taking my suggestion and pushing it as well – maybe you just want to add the ExecuteInternalVMTest and do the putback?

 

Many thanks Coleen

 


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 at HYPERLINK "http://cr.openjdk.java.net/%7Ecoleenp/8145940/"http://cr.openjdk.java.net/~coleenp/8145940/
bug link https://bugs.openjdk.java.net/browse/JDK-8145940
 
Thanks,
Coleen