Repeated IClassTransformer invocations

[Player3324]

As described in ForgeEssentials/ForgeEssentials#1351 or Chicken-Bones/ForgeMultipart#313 (similar issue), the current(?) implementation incorrectly runs transformers multiple times, potentially causing unexpected internal state there.

[Mumfrey]

I think you misunderstand the transformer contract. Transformers are not supposed to be stateful.

[Player3324]

That's wrong, there's no contract on the IClassTransformer implementation whatsoever. Stateless is an extra requirement over "nothing", just like thread safety or reentrant compatibility.

You don't match vanilla behavior.

[Player3324]

On a somewhat unrelated note, your own transformer is not reentrant, while it has to be. Your transformer code only doesn't end up in infinite recursion because it's stateful and excludes itself, but that's only sufficient in isolation, i.e. as long as nobody else does a similar thing.

If it was reentrant, it can't universally reliably react to external changes to the code being processed, which would be the only reason I can imagine why you are doing recursive transformation in the first place.

Btw. EntityPlayerMP is being run through the transformers 3 times, twice from TreeInfo.

[Mumfrey]

That's wrong, there's no contract on the IClassTransformer implementation whatsoever. Stateless is an extra requirement over "nothing", just like thread safety or reentrant compatibility.

I take your point, I was away at the time of your post so just had to boil the reply down to the most basic form I could. You're absolutely correct, there is no additional contract specified for transformers, which is the same reason why consumers can be allowed to not care about the internal state of transformer instances, it's not specified in the contract that they should, and thus from the point of view of consumers, a transformer is a "black box" which just takes in bytecode and outputs different (or the same) bytecode.

Transformers are of course perfectly at liberty to maintain internal state, in fact in many cases it's necessary. However just like any any other internal state when implementing an interface to the outside world it's up to the transformer itself to ensure that the state is preserved and sanity checked. This is what the mixin transformer does, because it's stateful but I don't put the onus on others to respect that, there are simply sanity checks in place so that if any assumptions prove to be wrong it becomes a detectable error condition which can then be reported and subsequently fixed (at my end).

To kind of wrap some semblance of sense around all this let me explain a little about the mixin system and hopefully we can figure out a sane way around this. Potentially explaining my motives and approach might yield some hitherto undiscovered solution:
 
Mixin loading effectively involves two separate class loading contexts (not really, but it's easiest to think of it like that), the normal classloading pipeline and the separate context in which the mixins themselves are loaded in order to be parsed. This second "context" runs all transformers on the loaded mixin code only, and by design builds a class transformer pipeline which excludes the mixin transformer itself. The actual intention of running the transformer pipeline is to allow the core FML and Forge transformers a chance to do any magic they require (such as deobf) on the mixin code, running other registered transformers is just a bonus and provides an extension point for transformers to actually run on mixin code before it's applied (which they normally wouldn't be able to).

In order to perform some of the more complex mixin transformations, such as updating static bindings, this context also needs information about target classes. However in order to obtain that information without interrupting the normal classloading order it loads the classes itself outside of the normal classloader (by reading the bytecode manually). It also runs them through the transformer chain so that any transformations which would be applied when the class is loaded "for real" are applied, primarily any changes which FML or Forge would make to the class are what we're after, just as for mixins.

Any metadata generated this way are cached, since essentially for any class in a mixin hierarchy we're going to end up "loading" the class twice (once for metadata, and once for real). Whilst this isn't really that efficient, it only happens for classes in a mixin hierarchy (which will be a small proportion of classes) and only at server startup when classes are loaded, so shouldn't affect overall server performance.

So there is some leeway here. Mixin classes are being run through the whole transformer chain (excluding the mixin transformer) but this could potentially be dealt with and maybe a good solution would be to provide an "opt out" for transformers which don't want to be run outside of actual classloading, this could be done via the Blackboard perhaps, I'm open to suggestions.

Creating second instances of transformers doesn't really gain us anything, especially with transformers whose "state" is actually maintained in other classes, but is definitely an option.

Btw. EntityPlayerMP is being run through the transformers 3 times, twice from TreeInfo.

That's a bug, and may be the real issue here. The expectation would be that the class is run through the transformer chain only twice (once in each context), so that's something I'll have to investigate, it's possible that something which should be being cached is not.

On a somewhat unrelated note, your own transformer is not reentrant, while it has to be. Your transformer code only doesn't end up in infinite recursion because it's stateful and excludes itself, but that's only sufficient in isolation, i.e. as long as nobody else does a similar thing.

Hopefully this makes a bit more sense now with the above explanation, I actually produced this diagram a while back which sort of explains the architecture I'm trying to achieve here:

If it was reentrant, it can't universally reliably react to external changes to the code being processed, which would be the only reason I can imagine why you are doing recursive transformation in the first place.

I'm not doing recursive transformation, quite the opposite. Recursion is a problem for the current approach which is why it's detected as an error condition (the intention is to fix this issue by the way, I just haven't reached that state in development yet, so the sanity check is the current safeguard so that things which are re-entrant can be avoided). The re-entrance check is not actually intended to catch unintended re-entrance from the mixin transformer itself, it's to catch unexpected re-entrance from other transformers (eg. a transformer which explicitly invokes Class.forName, which causes issues with parts of the current processor because I can't "save" a transformation state).

You don't match vanilla behavior.

It's not the intention to match vanilla behaviour. In all honesty this entire project would have been much easier to just integrate into a special ClassLoader, the mixin system is more of a ClassLoader level technology than a regular transformer, but for the sake of not breaking all the things it makes more (kind of hehe) sense to be a transformer, it's just that it's still doing class-loader-ey things but has to do them within the bounds of being a transformer. This has called for compromises and running the transformers twice is kind of one of them.

I'm happy to get ideas on this. Basically I'd be happy to scale it back to only run "critical" transformers (eg. ModSystem transformers such as those provided by FML, Forge, LiteLoader, etc.) but that would require some additional mechanism for obtaining which transformers are "critical". The other option is "opt out", which I dislike on principle (I don't like passing the buck as it were), but if a transformer author would rather opt-out of mixin processing than adding internal scaffolding to deal with maintaining internal state then I'm equally happy to accomodate that.

Appreciate any comments/ideas you have on the matter. In the mean time I'll investigate why TreeInfo is running the transformers twice when it should run them only once.

Sorry for the slow reply, I've been away the last couple of weeks.

[Mumfrey]

Just to add, I think I have worked out where the additional redundant (non-cached) class load/transform is happening and I can hopefully fix this now, so the number of transformer passes will return to 2 as intended.

[Mumfrey]

@sfPlayer1 do you have any further feedback on this. I'd like to take action on it if possible, or if no further action is required then I will close the issue for now. I'd like to try and resolve this one way or another.