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

[Liu, Xin]

hi, Volker, 

Here is a new revision with cloned(). 
http://cr.openjdk.java.net/~xliu/8249809/01/webrev/

thanks,
--lx

________________________________________
From: Volker Simonis <volker.simonis at gmail.com>
Sent: Tuesday, July 28, 2020 6:06 AM
To: Liu, Xin
Cc: 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 Tue, Jul 28, 2020 at 5:40 AM Liu, Xin <xxinliu at amazon.com> wrote:
>
> hi, Volker,
>
> Thank you to review my patch.
>
> 1. yes. I guess nodes are the major memory consumption, but compiler directives are for both c1 and c2.
> it still can reduce memory footprint a little.
>
> 2.  Previous code set the flag here. both ControlIntrinsic and DisableIntrinsic belong to compilerdirectives_common_flags.
>
>  327 #define init_default_cc(name, type, dvalue, cc_flag) { type v; if (!_modified[name##Index] && CompilerOracle::has_option_value(method, #cc_flag, v) && v != this->name##Option) { set->name##Option = v; changed = true;} }
>  328     compilerdirectives_common_flags(init_default_cc)
>
> When method-level directives override the global directives, this code snippet set changed.
> Even though I remove the flag 'changed', I have the same logic in the smart pointer.
>
> 3.  the smart pointer DirectiveSetPtr needs to provide 2 accesses of the underlying pointer.
> one is read-only and the other one is mutable.
>
> Ideally, it should has the following 2 operator->().
> DirectiveSet* operator->();
> DirectiveSet const* operator->().
>
> AFAFI, C++ doesn't support covariant return type overload.  That is to say, we need to find a way to work around.
> the reason I provide overload operator*() because it returns a reference to object.  Users who want to modify the pointee have to explicitly dereference the smart pointer.
> (*set).member = newvalue;
> set->member = newvalue; // compiler error.
>
> your approach also works,  but you need to invoke const_cast<> for all places where you want to  read.
> I think my approach has shorter code.
>
>
> I just came up a new idea.  How about I provide a method cloned(), which returns the unqualified pointer?
>
> +  DirectiveSet* cloned() {
> +    if (!_clone) {
> +      _clone = DirectiveSet::clone(_origin);
> +    }
> +    return _clone;
> +  }
> +
>    DirectiveSet* transfer() {
>      assert(_origin != NULL, "_origin is NULL! transfer() can only be invoked once.");
>      if (_clone != NULL) {
> @@ -340,7 +347,7 @@
>
>      if (CompilerOracle::should_print(method)) {
>        if (!_modified[PrintAssemblyIndex]) {
> -        (*set).PrintAssemblyOption = true;
> +        set.cloned()->PrintAssemblyOption = true;
>        }
>      }
>

Hi Xin,

I like this solution much better. It makes it clear that we want to
alter the state of the Directive Set. Can you please provide a new
webrev based on this idea?

Thank you and best regards,
Volker

> thanks,
> --lx
>
> ________________________________________
> From: Volker Simonis <volker.simonis at gmail.com>
> Sent: Monday, July 27, 2020 8:51 AM
> To: Liu, Xin
> Cc: 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.
>
>
>
> Hi Xin,
>
> I'm not sure if saving the allocation of an DirectiveSet has any
> visible effect compared to the much larger allocations required for
> the method compilation itself.
>
> Apart from that, I must confess that I'm not totally understanding the
> original logic. From what I see, it sets "changed" to true in the case
> where it changes the cloned DirectiveSet. But it doesn't do that in
> the cases where it only changes the clone's control word:
>
>  341         set->_intrinsic_control_words.fill_in(TriBool());
> ...
>  348           set->_intrinsic_control_words[id] = iter.is_enabled();
> ...
>  361         set->_intrinsic_control_words.fill_in(TriBool());
> ...
>  368           set->_intrinsic_control_words[id] = false;
>
> Why don't these mutations count as "changing"  the cloned DirectiveSet?
>
> After your patch, you've changed the above lines such that they will
> always create a clone which seems different from the initial
> behaviour.
>
> Which of the two behaviours is correct here, the original one, the new
> one after your change or doesn't it matter for reasons I don't
> understand?
>
>
> I also wonder why you need to overload both operators "operator*()"
> and "operator->()"? It seems a little bit arbitrary (and hard to
> understand for people reading the code) that "operator*()" clones the
> underlying directiveSet while "operator->()" uses the original one.
> Why not just define two versions of "operator->()" and let the
> compiler choose the right one like so:
>
> DirectiveSet const* operator->() const {
>   return !_clone ? _origin : _clone;
> }
>
> DirectiveSet* operator->() {
>   if (!_clone) {
>     _clone = DirectiveSet::clone(_origin);
>   }
>   return _clone;
> }
> ...
> if (!_modified[LogIndex]) {
>   bool log = CompilerOracle::should_log(method);
>   if (log != const_cast<const DirectiveSetPtr&>(set)->LogOption) {
>     set->LogOption = log;
>   }
> }
>
> Thank you and best regards,
> Volker
>
> On Mon, Jul 27, 2020 at 1:47 AM Liu, Xin <xxinliu at amazon.com> wrote:
> >
> > hi, Reviewers,
> >
> > Could you review this simple patch?
> > bug: https://bugs.openjdk.java.net/browse/JDK-8249809
> > webrev: https://cr.openjdk.java.net/~xliu/8249809/00/webrev/
> >
> > When the users specify a method-level compiler directive, the DirectiveSet is cloned for every single compiling method. It's expensive but rarely hit. Actually, Only user-specified methods must clone the DirectiveSet. I introduce a smart pointer DirectiveSetPtr. operator->() returns a pointer to a constant DirectiveSet, which is read-only. It doesn't clone the _origin until c2 need to update its members. transfer() yield the ownership of the pointer.
> >
> > Test:
> > manually tests with different CompileComand options.
> > hotspot:tier1 and gtest:all.
> >
> > thanks,
> > --lx
> >