Groovy code using constants seems unsuitable for hotswap

So, it's not really specific to constants such as "const$..", is it?

If I have

class A {
    static a = 1
    static b = 2
    static c = a
}

and change "c = a" to "c = b", even its hotswap should be affected, right?

Enum constants are also initialized in <clinit>.

I am wondering whether all field initialization should move to a helper method that gets called from <clinit> and not just the "const$.." fields' initialization.

No it is not specific to const$ but in the short term I would initially just go with the Java flow on this so that expectations are the same for users coming to groovy. I wouldn't expect changing c=a to c=b to be picked up on hotswap (basically I wouldn't expect static state to change across hotswaps). But I would expect changes in an instance method to be picked up. Running any kind of helper method during a hotswap event is unusual I would say - I'm just already having to do that because of the call site caches, so if I have to do that to pickup constant value changes in instant methods I can. It isn't meant (right now) to be a general solution to picking up static state change.

Can I ask why constants are created for these instance method values? What is the benefit?

Since Groovy stores only objects in the method frame it is desireable to cache those objects somehow. This is done using those constants. For example

a=1
b=1

should create only one constant, that stores ne Integer(1) or the valueOf version. In the end the benefit is to save cost for boxing.

Hi, I am attaching here a patch that pulls the initialization of $const$.. constants in a separate method and calls that from the static initializer instead of doing it inline in the initializer.

Please let me know if it will help.

Hi, anyone got a chance to review the patch?

I am not really satisfied with the patch. First thing is that the method in ClassNode should be in Verifier, but that is only a side thing. Instead of creating the const fields and then move them later I am thinking that it might be better to not to use the constants at all.

I am thinking of creating a hotswap profile, that allows some adaptions to the compiler to allow hotswapping. That way we can do much more in different parts of the compiler. Of course that would be a much bigger thing to do

Why is not using the constants better than moving them into a method? Are they not really bringing any benefit that is worth keeping them?

They help reducing the cost for boxing. That is something I think we can sacrifice. My goal would be to make a Groovy class as "hotswap able" as a Java class in this profile. That includes no call site caching specialties and no constants, that don't need to be there.

If you do decide to remove them, I would look to perhaps altering your boxing calls slightly. Right now you call 'new Integer' (etc). It is usually better to call Integer.valueOf since that creates/caches and shares Integer representations for some values (-128 > +127)
(Integer.valueOf is only since 1.5 though, if that makes any difference). This can reduce the amount of object creation (and is also the way javac boxes)

Jochen, since this issue/patch has come up on the mailing list today, I thought I should check:

• have u started anything on the goal of having a groovy class as "hotswap able" and do the profile specific things - in one profile, having all callsite caching, consts optimization, but not in the other profile? If we are not close to that design yet, may be in the meantime, how about going ahead with the patch?

• The patch has some changes in ACG/verifier, etc and you have mentioned that you have been re-designing ACG - do u see any conflicts? When is your re-done ACG coming out? Maybe the patch should wait for it and then get reworked.

I have not tried anything really.... The problem is there is no real documentation to this. I asked JVM developers and they just looked scared

My redone ACG is coming out as soon as I all tests pass... which I am much nearer than a week ago, but still a bit far away (lot's of VerifyErrors still). Don't worry about conflicts with ACG, the changes to ACG are so big that I will have to include every patch by hand anyway.

Now... the patch here still has the problem with having a method on ClassNode which should be in Verfier, but I still think having an option to those elements not adding this stuff is much more easy. But let us think about setting that in a method... why not do that in general and change the places that contribute to that?

In that regard CallSite caching will be a problem, since that happens in ACG and that init method would need to be processed last.

Created GROOVY-4412 for the new Integer() vs Integer.valueOf() issue as it can be solved independently of this issue, as it is bothering other users as well who use FindBugs for code analysis that includes groovy classes.

I make this a blocker, since this should go into the next release. Later we can reduce the priority again.

I think now, that Roshan's idea is the way to go. Besides the constants we have also to null out the call site array field, otherwise hotswapping may cause Groovy to pick up the wrong method.

1.8 code to fix that can be found in r21437 Tests and 1.7 megre still missing

r21442, 1.7 merge done