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

[Coleen Phillimore]

On 1/11/16 4:18 PM, Markus Gronlund wrote:
>
> 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?

Sorry, I meant if I made the copy constructor private, I got a 
compilation error.  It doesn't have to call it though.
> 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?

Yes.   This form does not call the copy constructor.    But since 
SymbolTable::new_symbol("abc", CHECK) takes a TRAPS argument, you can't 
specify it this way because of the CHECK expansion.

> 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 
> <http://cr.openjdk.java.net/%7Emgronlun/8145940/8145940_TempNewSymbol_analysis.txt>  
> Webrev suggesting new impl:http://cr.openjdk.java.net/~mgronlun/8145940/webrev01/ 
> <http://cr.openjdk.java.net/%7Emgronlun/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.
>
> [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?
>

Okay, I'll patch in your change instead of mine and resend it with my 
ExecuteInternalVMTest.

Thanks!
Coleen
>
> 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 athttp://cr.openjdk.java.net/~coleenp/8145940/ 
> <http://cr.openjdk.java.net/%7Ecoleenp/8145940/>
> bug linkhttps://bugs.openjdk.java.net/browse/JDK-8145940
> Thanks,
> Coleen
>