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

[Volker Simonis]

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