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

[Volker Simonis]

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
>