[Suggestion] Skipping execution if possible in ModifyExpressionValue

[altrisi]

In a regular Java conditional, if you do something like this:

if (cheapCheck() && expensiveCheck())

expensiveCheck() will only run if cheapCheck() evaluates to true (and with || only if false).

However, with ModifyExpressionValue, as specified in the wiki, an injector like this turns into the following:

    @ModifyExpressionValue(method = "someMethod", at = @At(
            value = "INVOKE",
            target =  "LFoo;expensiveCheck(LParameters;)Z"
    ))
    private boolean cancelIfPossible(boolean original)
    {
        return cheapCheck() && original;
    }

if (cancelIfPossible(expensiveCheck()))

This forces expensiveCheck() to run every time, given it is passed as a parameter and therefore can't ever be skipped (unless it has no side effects or something, I'm not an expert on this).

Would it be possible for MixinExtras to inline those in order to allow such checks to be skipped? (note that I have no idea if this is actually possible or feasible, just asking) Or would it cause the same incompatibilities as Redirect?

An equivalent Redirect that would do the job would be something like this:

    @Redirect(method = "someMethod", at = @At(
            value = "INVOKE",
            target =  "LFoo;expensiveCheck(LParameters;)Z"
    ))
    private boolean cancelIfPossible(Foo object, Parameters params)
    {
        return cheapCheck() && object.expensiveCheck(params);
    }

[altrisi]

Hmm may not be possible if the method isn't a simple expression but something more complicated (note that I still don't know about this, just thought about it and decided to add it to the issue, sorry if it's useless)

[LlamaLad7]

This is a tricky one, because inlining anything from a handler is a lot more complicated than it might first appear, especially when there are jumps involved, and usages of things which are already on the stack. A more general idea I had was some kind of redirect which would give you some way to call what was there before (whether that be the original method or another redirect), e.g. by giving you a reference to it. Alternatively some kind of CallbackInfo-like object specifically for a single call in a wider method.

However I've given a lot of thought to this, and it will be quite tricky to implement in a way that is compatible with not only other usages of "new redirect"s, but also usages of mixin's own redirects. As such, this would be more of a long term idea, and I'll need to put some more thought into it.

[florensie]

No need for a CallbackInfo-like object:

@ModifyExpressionValue(method = "someMethod", at = @At(
        value = "INVOKE",
        target =  "LFoo;expensiveCheck(LParameters;)Z"
))
private boolean cancelIfPossible(Supplier<Boolean> original) {
    return cheapCheck() && original.get();
}

Before:

- if (cheapCheck() && expensiveCheck())
+ if (cheapCheck() && cancelIfPossible(expensiveCheck()))

After:

- if (cheapCheck() && expensiveCheck())
+ if (cheapCheck() && cancelIfPossible(() -> expensiveCheck()))

The implementation does worry me a little, you'd have to generate the lambda/synthetic method and I'm not sure how easy that is, considering "out of scope" variables might be used.

[LlamaLad7]

Sure, that's a nice implementation from the user's point of view, but it becomes complicated when trying to chain them, especially when trying to chain them with existing normal-style redirects. I'll have a think about how would be best to do this and take all of the options into account.

[TheEpicBlock]

The JVM does inlining by itself. Considering these methods are really small I doubt the JVM wouldn't inline these.

[altrisi]

Yes, but the code would still be ran, because the intention is to have the side effects (it's evaluating before entering the function). Else it could change behavior.

[LlamaLad7]

Will be solved by https://github.com/LlamaLad7/MixinExtras/wiki/WrapOperation

[LlamaLad7]

Now stable in 0.1.0, which marks the completion of this goal.