RFR[XS] 8249809 avoid calling DirectiveSet::clone(this) in compilecommand_compatibility_init

[Liu, Xin]

hi, Volker, 

Your suggestion is great. I took it.  The assertion is there because I want to prevent a pointer from releasing more than one time. 
the downside is I limit how to use the function transfer(). 

I just came up a new idea. I changed the function name from transfer() to commit(). 
if _clone is not nullptr, commit() will overwrite _origin and reset itself to nullptr.  cloned() provisions a new object to update. commit() finalizes it.  

it's exaggerated,  but we can use the smart pointer repeat. 
+    set.commit(); // update _origin
+    set.cloned();  // clone it again
+    set.commit(); // update _origin again
+    set.commit(); // no-op
+    set.cloned();  // clone a new one. 
+    set.cloned();  // no-op
     return set.commit();

here is the new revision: 
https://cr.openjdk.java.net/~xliu/8249809/03/webrev/

thanks,
--lx



________________________________________
From: Volker Simonis <volker.simonis at gmail.com>
Sent: Thursday, July 30, 2020 10:03 AM
To: Liu, Xin
Cc: Tobias Hartmann; hotspot-compiler-dev at openjdk.java.net
Subject: RE: [EXTERNAL] RFR[XS] 8249809 avoid calling DirectiveSet::clone(this) in compilecommand_compatibility_init

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



On Wed, Jul 29, 2020 at 10:56 PM Liu, Xin <xxinliu at amazon.com> wrote:
>
> hi, Volker and Tobias,
>
> Here is a new revision.
> http://cr.openjdk.java.net/~xliu/8249809/02/webrev/
>
> 1. This one add comments about this smart pointer and fix the formation issue.
>
> 2. Thanks to point me out a new document of hotspot code style.
> Since it has updated to -std=c++14, I change all NULL to nullptr.
>
> 3.  I also add NON_COPYABLE because it's not intended to be copied.
>
>
> DirectiveSetPtr is just a thin wrapper of the raw pointer. if users only use it to read,  nothing will be cloned. It simply goes through.
>

Hi Xin,

I like the new version :)

I think it's fine except the assertion in "transfer()":

 308   DirectiveSet* transfer() {
 309     assert(_origin != nullptr, "_origin is NULL! transfer() can
only be invoked once.");
 310
 311     if (_clone != nullptr) {
 312       // We are returning a (parentless) copy. The original
parent don't need to account for this.
 313       DirectivesStack::release(_origin);
 314       _origin = nullptr;
 315       return _clone;
 316     }
 317     else {
 318       return _origin;
 319     }
 320   }
 321 };

You should either move it into the " if (_clone != nullptr)" block or
set "_origin" to NULL in the "else" branch as well.

Best regards,
Volker

PS: I won't have access to mail for the next two weeks. If there won't
be any fundamental changes to this patch any more you can consider it
reviewed from my side.

> thanks,
> --lx
>
> ________________________________________
> From: Volker Simonis <volker.simonis at gmail.com>
> Sent: Wednesday, July 29, 2020 7:34 AM
> To: Tobias Hartmann
> Cc: Liu, Xin; hotspot-compiler-dev at openjdk.java.net
> Subject: RE: [EXTERNAL] RFR[XS] 8249809 avoid calling DirectiveSet::clone(this) in compilecommand_compatibility_init
>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Wed, Jul 29, 2020 at 9:38 AM Tobias Hartmann
> <tobias.hartmann at oracle.com> wrote:
> >
> > Hi Xin,
> >
> > On 28.07.20 22:56, Liu, Xin wrote:
> > > http://cr.openjdk.java.net/~xliu/8249809/01/webrev/
> >
> > Overall looks good to me.
> >
> > Some style comments:
> > - Add a comment to 'DirectiveSetPtr' to describe its purpose
> > - Why not put the "cloned" logic in "operator->"?
>
> Because there's also a "read-only" access  of the DirectiveSetPtr
> which doesn't mutate its content and therefore should clone the
> underlying DirectiveSet. See my first mail where I proposed to add a
> second, `const`-version of "operator->". But that still required const
> casts in the places where we didn't want to clone. I've therefore
> voted for the new "cloned()" method which makes cloning and mutating
> explicit and which is much easier to understand from my point of view
> (compared to two overloaded operators).
>
> > - Do not use the _clone pointer as boolean (see "Miscellaneous" section in the style guide [1])
> > - Indentation in line 301-303 is wrong
> > - Line 306 use brackets around the "else" and move it one line up "} else {"
> >
> > Best regards,
> > Tobias
> >
> > [1] https://hg.openjdk.java.net/jdk/jdk/raw-file/tip/doc/hotspot-style.html