blackdrag commented on PR #2736:
URL: https://github.com/apache/groovy/pull/2736#issuecomment-5108446270
> @blackdrag Thank you for pressing on the “when / why” of hierarchy
fan-out. That is the right question, and the previous reply was still too
coarse. Below is the refined model after re-checking MOP selection against your
scenario.
Now I feel kind of bad, because I made a major mistake in my example. not
`A extends B extends C and call(C x) { x.fooC() } with c = new C():` In my
head the hierarchy was reversed. The goal was specifically to see how changes
in the metaclasses of super classes affect the child metaclass. Sorry for that.
[...]
> ## 2. Why hierarchy exists at all (and when it does not)
>
> Hierarchy fan-out is **not** “`MetaClassImpl` shares one table up the
hierarchy”. Each class still has its own MetaClass / `ClassInfo` domain.
>
> It exists because **selection** can observe _ancestor_ MetaClass state:
>
> * `MetaClassImpl.findMethodInClassHierarchy` only opens a real walk
when some strong MetaClass in the hierarchy is a **modified**
`MutableMetaClass` (EMC is the common case).
>
> * Empirically, `Parent.metaClass.hello = {…}` is visible on `new
Child().hello()` **without** `enableGlobally()`.
>
> * Empirically, `Object[].metaClass.arrHello = {…}` is visible on
`String[]` — that is why the array lattice is indexed.
but the important point here is that this happens only for a missing method.
And it is actually why I think we are having a major semantic gap here, that we
have to decide on:
```
class A{}
class B extends A{
def m1(){1}
def m2(Integer x){2}
}
class C extends A {
def m2(x) { 40 }
}
def c = new C()
// normal calls
def b = new B()
assert b.m1() == 1
assert b.m2(0) == 2
// methods added using EMC, including overload
A.metaClass.m1 = {-> -1}
A.metaClass.m2 = {Integer x-> -2}
A.metaClass.m2 = {String x-> -3}
def a = new A()
assert a.m1() == -1
assert a.m2(0) == -2
assert a.m2("") == -3
// setting metaclass again, but no change
b.metaClass = null
assert b.m1() == 1
assert b.m2(0) == 2
// this method did not exist before, now found by
// MetaClassImpl.findMethodinClassHierarchy
// BUT no valid callsite for this before did exist -> no invalidation case
assert b.m2("") == -3
// even though m2(Integer) and m2 (String) exist in the metaclass for A
// they are not used for C, because the metaclass for C existed before
// they have been added!
assert c.m2(null) == 40
assert c.m2(0) == 40
assert c.m2("") == 40
// reset meta class, still the same, because MetaClassImpl is cached
c = new C()
assert c.m2(null) == 40
assert c.m2(0) == 40
assert c.m2("") == 40
// is now EMC
c.metaClass.m1 = {-> -40}
assert c.m1() == -40
assert c.m2(null) == 40
assert c.m2(0) == -2
assert c.m2("") == -3
// same structure as C, but instance exists *after* A has been modified!
// D is still MetaClassImpl
class D extends A {
def m2(x) { 50 }
}
d = new D()
assert d.m2(null) == 50
assert d.m2(0) == -2
assert d.m2("") == -3
```
The case of C and D show how broken the current system is. The point of time
a MetaClassImpl instance comes into existence should not decide about what it
sees. In fact my POV is that MetaClassImpl should be immune to changes of other
meta classes. This means especially that no hierarchy check is needed for it
and that find method should not exist on MetaClassImpl. Now to produce a
version with callsite caching:
```
import org.codehaus.groovy.runtime.metaclass.*
class A{}
class C extends A {
def m2(x) { 40 }
}
def call(x,y) {
x.m2(y)
}
def c = new C()
A.metaClass.m2 = {Integer x-> -2}
A.metaClass.m2 = {String x-> -3}
// no methods from A visible
assert call(c,0) == 40
c = new C()
assert call(c,0) == 40
def mc = C.metaClass.delegate
assert mc instanceof MetaClassImpl
// is now EMC enforces methods from A
c.metaClass.m1 = {-> -40}
assert call(c,0) == -2
// reset to old MC
c.metaClass = mc
assert call(c,0) == 40
// simulate late init, still MetaClassImpl, but now change is visible
def registry = MetaClassRegistryImpl.getInstance(0)
registry.removeMetaClass(C)
c.metaClass = null
assert call(c,0) == -2
def newMc = c.metaClass.delegate
assert newMc instanceof MetaClassImpl
assert newMc != mc
// reset meta class of A, but old methods from A still visible
A.metaClass = null
assert call(c,0) == -2
// show that A does not know m2
def catched = false
try {
new A().m2(0)
} catch (Exception e) {
catched = true
}
assert catched
// now we switch to the old emc, which is clueless about A
c.metaClass = emc
call(c,0) == 40
```
Most of this should not be caught by the switch point, but by the fact, that
the metaclass changed. But I also think I captured cases where a hierarchy
version would break the code in case of MetaClassImpl. Also I think it should
behave the same if classes used are Java based and not from Groovy.
[...]
> This matches your intuition: **without EMC (and without a modified mutable
MetaClass), hierarchy is unnecessary**. Pure `MetaClassImpl` parent replace no
longer fans out to children. Unknown/custom MetaClass kinds still fan out
(correctness-first).
+1
> ## 3. “Should the MetaClass own the SwitchPoint?”
>
> Agreed in principle. The current class-domain SP is a **stand-in for
class-level MetaClass generation**, not a claim that every MetaClass kind
shares the same invalidation logic.
>
> A MetaClass-owned guard would be the natural place for:
>
> * MetaClass-specific update vs replace semantics,
> * true per-instance domains (today: uncacheable PIC + exact class
retire),
> * custom MetaClass implementations that do not fit EMC /
`MetaClassImpl`.
>
> That is a larger redesign than this PR. For 6.0 we kept a single
monomorphic hot-path guard and made invalidation **MetaClass-aware** at the
registry boundary. MetaClass-owned SwitchPoints remain a follow-up once the
“when / why” matrix above is agreed.
ok, agreed... if a follow-up issue is created
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
