RFR (xs) 8145940: TempNewSymbol should have correct copy and assignment functions
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Jan 13 20:11:00 UTC 2016
Okay, thanks! I'll reword as suggested.
Coleen
On 1/13/16 3:01 PM, Markus Gronlund wrote:
> I think it looks good.
>
> I agree with adding in Lois's comment about the assignment operator.
>
> No need for another webrev.
>
> Thanks
> Markus
>
> -----Original Message-----
> From: Lois Foltan
> Sent: den 13 januari 2016 19:21
> To: Coleen Phillimore
> Cc: Markus Gronlund; hotspot-runtime-dev
> Subject: Re: RFR (xs) 8145940: TempNewSymbol should have correct copy and assignment functions
>
>
> On 1/13/2016 11:28 AM, Coleen Phillimore wrote:
>> Hi, Thanks Kim, Markus and Lois. This is the latest version that I've
>> retested and added to the internal test:
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8145940.03/
> I think this looks good. A few minor comments:
>
> - symbolTable.cpp - you could have the test case contain a
> Symbol*/TempNewSymbol* defined within an inner scope to ensure the correct ref counting happens within the inner scope and upon exit from the inner scope.
>
> - symbolTable.hpp - Glad to see the comment ahead of the assignment operator. I find the last sentence a bit confusing. Can you rephrase to something like, "rhs is passed by value, within the scope of this method it is a copy. At method exit it contains the former value of _temp, triggering the correct refcount decrement upon destruction."
>
> I don't need to see another webrev.
>
> Thanks,
> Lois
>
>> Thanks,
>> Coleen
>>
>> On 1/13/16 5:35 AM, Markus Gronlund wrote:
>>> Hi Coleen,
>>>
>>> The Test_TempNewSymbol() look fine - thank you for adding it.
>>>
>>> Maybe you could also complement with a simple self-assignment test as
>>> well?
>>> ...
>>>
>>> s1 = s1; // self assignment
>>> assert(s1->refcount() == 2, "should still be two abc (s1 and
>>> ss)");
>>>
>>> ...
>>>
>>> Thanks!
>>> Markus
>>>
>>>
>>> -----Original Message-----
>>> From: Coleen Phillimore
>>> Sent: den 12 januari 2016 22:59
>>> To: Markus Gronlund; Kim Barrett
>>> Cc: hotspot-runtime-dev
>>> Subject: Re: RFR (xs) 8145940: TempNewSymbol should have correct copy
>>> and assignment functions
>>>
>>>
>>>
>>> On 1/12/16 7:34 AM, Markus Gronlund wrote:
>>>> Hi Kim,
>>>>
>>>> Thanks a lot for the (detailed) reminder about the copy-and-swap
>>>> idiom. I agree with your reasoning.
>>> Two for (strongly?) and two against (myself, not so strongly and we
>>> can always add a comment to save someone else's time). I can change
>>> it (hoping my test still pass). Did you look at the test?
>>>> About the return value from operator=, I would suggest retaining the
>>>> void as-is as well.
>>> Ok
>>>> Maybe we should investigate if it is possible to have the functions
>>>> in SymbolTable that increment Symbol*'s return a TempNewSymbol( or
>>>> some other "smartptr" concept) instead of a direct Symbol*?
>>> When changing symbolOops symbolHandles, to Symbol* with refcounting I
>>> had an implementation using SymbolHandle as a smart pointer. I think
>>> I had the assignment operator transfer ownership of the pointer,
>>> iirc. I had added reference counting statistics at that time (which
>>> I have
>>> output in my RFR). My memory was that the number of Atomic:: add and
>>> subtract operations were an order of magnitude larger. I saved a lot
>>> of email on this except the numbers. Symbols mostly come into the
>>> JVM through the constant pool, with these exceptions that use
>>> TempNewSymbol.
>>>
>>>> In that case we could make the type conversion constructor private
>>>> and make SymbolTable a friend of TempNewSymbol - this would avoid
>>>> accidental wrapping of arbitrary Symbol*'s. I will look into the
>>>> possibility of this.
>>> This sounds like a good idea.
>>>
>>> Thanks,
>>> Coleen
>>>> Thanks again for your clarification
>>>> Markus
>>>>
>>>> -----Original Message-----
>>>> From: Kim Barrett
>>>> Sent: den 12 januari 2016 05:22
>>>> To: Coleen Phillimore
>>>> Cc: hotspot-runtime-dev
>>>> Subject: Re: RFR (xs) 8145940: TempNewSymbol should have correct
>>>> copy and assignment functions
>>>>
>>>> On Jan 11, 2016, at 8:34 PM, Coleen Phillimore
>>>> <coleen.phillimore at oracle.com> wrote:
>>>>> Kim and Lois,
>>>>>
>>>>> Thanks for looking at this. This is my latest webrev with the
>>>>> ExecuteInternalVMTest in it:
>>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/8145940.02/webrev/index.html
>>>>>
>>>>> See comments below.
>>>>>
>>>>> On 1/11/16 6:45 PM, Kim Barrett wrote:
>>>>>> On Jan 7, 2016, at 3:02 PM, Coleen Phillimore
>>>>>> <coleen.phillimore at oracle.com> wrote:
>>>>>>> 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
>>>>>> For the assignment operator, I think better is:
>>>>>>
>>>>>> void operator=(TempNewSymbol s) {
>>>>>> Symbol* tmp = s._temp;
>>>>>> s._temp = _temp;
>>>>>> _temp = tmp;
>>>>>> }
>>>>>>
>>>>>> This is the well-known copy and swap idiom, with a by-hand "swap".
>>>>>>
>>>>>> http://stackoverflow.com/questions/3279543/what-is-the-copy-and-sw
>>>>>> ap
>>>>>> -
>>>>>> idiom
>>>>> Very tricky. Lois and I figured out the trick. It doesn't elide
>>>>> any Atomic::* reference counting though, which is what is worrysome
>>>>> for performance, just hides it in the destructor of 's'. I don't
>>>>> want to change this.
>>>> Copy-swap saves several refcount manipulations in some (common)
>>>> cases, by taking advantage of rvalue copy elision.
>>>>
>>>> Coleen's:
>>>>
>>>> void operator=(const TempNewSymbol& s) {
>>>> clear();
>>>> _temp = s._temp;
>>>> if (_temp != NULL) _temp->increment_refcount(); }
>>>>
>>>> Markus's:
>>>>
>>>> const TempNewSymbol& operator=(const TempNewSymbol& rhs) {
>>>> if (this != &rhs && _temp != rhs._temp) {
>>>> clear();
>>>> _temp = rhs._temp;
>>>> if (_temp != NULL) {
>>>> _temp->increment_refcount();
>>>> }
>>>> }
>>>> return *this;
>>>> }
>>>>
>>>> Kim's:
>>>>
>>>> void operator=(TempNewSymbol rhs) {
>>>> Symbol* tmp = rhs._temp;
>>>> rhs._temp = _temp;
>>>> _temp = tmp;
>>>> }
>>>>
>>>> [I'm ignoring differences in the return value in the discussion below.
>>>> I think it ought to be either "TempNewSymbol&" (not const) or void.]
>>>>
>>>> Markus's is mostly the same as Coleen's, except it handles a couple
>>>> of special cases differently.
>>>>
>>>> Markus's handles self-assignment, where Coleen's doesn't. For
>>>> correctness that's important; in actual practice self-assignment is
>>>> generally rare to non-existent, so always paying overhead for it is
>>>> unpleasant.
>>>>
>>>> Markus's also specially handles re-assignment to the same symbol,
>>>> avoiding any refcount changes in that case. I suggest this is
>>>> another rare case, and adding overhead in the normal case to get
>>>> that rare benefit is not the right tradeoff.
>>>>
>>>> Assuming we're not in one of those special cases and neither Symbol*
>>>> is NULL, Coleen/Markus decrements the old and increments the new.
>>>> And in an important special case where rhs is an rvalue, there will
>>>> be another decrement as the temporary goes out of scope. So two
>>>> refcount changes when rhs is an lvalue, three when it's an rvalue.
>>>>
>>>> In contrast, Kim's does an increment and a decrement when rhs is an
>>>> lvalue, and *only* a decrement when rhs is an rvalue and rvalue copy
>>>> elision is performed by the compiler, which any decent optimizing
>>>> compiler will do.
>>>>
>>>> The rhs is an rvalue in the following situation:
>>>>
>>>> TempNewSymbol tns = ... ;
>>>> ...
>>>> tns = expr_returning_Symbol* ;
>>>>
>>>> The rhs gets implicitly converted to a temporary (rvalue)
>>>> TempNewSymbol referring to the symbol (no refcount change), that
>>>> temporary is directly bound to the assignment argument (copy
>>>> elided), swap the symbols, and the rvalue converted temp goes out of
>>>> scope and decrements the refcount of the old Symbol*.
>>>>
>>>> The copy-swap version also handles the (rare) self-assignment and
>>>> re-assignment to the same symbol cases. In these cases Kim's still
>>>> does the same refcounting changes as otherwise. But it's better to
>>>> do extra work in the rare cases and keep the normal cases fast.
>>>>
>>>> This is why copy-swap has become the "usual idiom"for assignment
>>>> operators. There might be cases where it's not the best answer, but
>>>> usually it is. And this is why (to answer Lois's issue) one should
>>>> use it even though it's not obvious if one is not previously
>>>> familiar with the idiom; one should instead learn the idiom, and
>>>> then one spends time looking hard at implementations that do
>>>> something else.
>>>>
>>>> Of course, this may all be entirely in the noise; I hope that
>>>> TempNewSymbol assignment is not a high frequency operation.
>>>>
>>>>
>>>>>> I think the updated description of TempNewSymbol isn't really right.
>>>>>> Because the conversion constructor doesn't increment the reference
>>>>>> count, it must *not* be used to capture some arbitrary reference
>>>>>> to a Symbol*. Only a new symbol or one obtained from one of the
>>>>>> lookup functions really should be managed by this class.
>>>>> Yes, this is a subtlety of the design and one that I can't really
>>>>> fix right now. I thought of changing TempNewSymbol to convert from
>>>>> const char * but there are cases where we don't want to add a symbol.
>>>>>
>>>>> There is a comment above TempNewSymbol. I don't know what to add
>>>>> to it. And Markus's comments are really good.
>>>> I was confused about which webrev I was looking at. Coleen's
>>>> doesn't seem to change those comments.
>>>> It was (parts of) Markus's comments that worried me. And the whole
>>>> design is tricky and risky.
>>>>
More information about the hotspot-runtime-dev
mailing list