MINOR: Cache required arg count in CompiledMethodIR

[original-brownbear]

The required arg count is constant for a compiled method:

• Made all steps of getting this number final (they obviously are final already in the execution)
• cached the required count
  • Had to work around the fact that the Signature is null at times (in which case the required number isn't used down the line)

Saves us 3 bytes on this method, which can't hurt here? :)

left is this version, right is master

[enebo]

@original-brownbear I think doing this savings is good but I am curious if you can dig and figure out why we don't always pass in signature. I propose that if we can fix always passing in signature and we change frobnicateKwargsHash to accept specificArity instead of a new required field we can kill those bytecodes, remove some extra conditional logic, and kill having an additional new field.

Another interesting wrinkle coming is that we plan on adding calling conventions through our code paths. Right now all args[] are straight up argument lists but in the future we will add a new call convention which are keyword arguments inlined into args[] as positional arguments (with a marker object on end to determine it is a special kwarg calling convention -- at least this is the most popular idea atm). This will not likely mean a ton for *Method but it will mean we will not be calling frobnicateKwargsHash in those cases since something further up the line will more efficiently process these arguments without making Ruby hashes per call.

We had started replacing Arity with Signature in the past and I think we got stalled once we landed in maybe JavaMethodX? Signature was meant to kill Arity eventually but we did not make it before 9k needed to get out. (history :) )

[original-brownbear]

@enebo

I am curious if you can dig and figure out why we don't always pass in signature. I propose that if we can fix always passing in signature and we change frobnicateKwargsHash to accept specificArity instead of a new required field we can kill those bytecodes, remove some extra conditional logic, and kill having an additional new field.

Sounds like a plan, will look into that soon (either later today or tomorrow :))

[enebo]

@original-brownbear and I should mention that specificArity and its relationship to frobnicate line is a little weird. I would even be willing to just pass signature into frobnicate (I think JIT also calls that so we may need a second method since that will definitely favor an int vs object). frobnicate is the only show in town for kwargs but as I say it will be destined for the non-fast path of kwargs in the future so calling required() there will be a drop in the bucket (compared to making multiple ruby hashes).

[original-brownbear]

@enebo

I think doing this savings is good but I am curious if you can dig and figure out why we don't always pass in signature

The answer is actually in the JDoc :) of org.jruby.parser.StaticScope#getSignature :)

    /**
     * For all block or method associated with static scopes this will return the signature for that
     * signature-providing scope.  module bodies and other non-arity specific code will return null.
     */
    public Signature getSignature() {
        return signature;
    }

frobnicate is the only show in town for kwargs but as I say it will be destined for the non-fast path of kwargs in the future so calling required() there will be a drop in the bucket (compared to making multiple ruby hashes).

In that spirit, how about this:
e94eb2b

just make the signature a protected to save the internal getter call, pass Signature to an overload of frobnicateKwargsArgument and split out the slow path in frobnicateKwargsArgument.

• same initial method size as with caching required
• frobnicateKwargsArgument becomes a little larger but as you said it's irrelevant there
  • But just in case, how about my approach of separating the non-breakout part of that method to get nicer inlining in some cases? :)

[enebo]

@original-brownbear I think e94eb2b looks good and I realized after I sent my comment frobnicate and signature are not neccesarily one task. Your commit does completely ignore needing to know whether it is null (since it is only used in a case where we know there is a signature because it contains kwargs).

As follow up perhaps we should consider whether we should be using CompileIRMethod for scope bodies which are not methods OR passing in a null pattern for no-arg signatures. Former seems clearer but adds maintenance issues and the later perhaps is not helpful (I mean an NPE sort of makes sense if you think you need to reason about arg lists in a module body....BUT...we did not even remember why signature could be null in the first place so an NPE is an unclear error). I think one could argue that a non-block/method body is a required 0 signature too.

[enebo]

@original-brownbear I will merge after ci is done but I will probably remove final from method getSignature. The number of times Java's internal APIs have marked a method final and screwed my ability to override it in a subclass is enough where I am not a fan. tbh I am a bit final adverse in general but I like pointing out places where it is only used in an immutable context (even though that is not what it means).

[original-brownbear]

@enebo hmm sorry for the back and forth, but I just thought of a much cleaner alternative here actually ... what do you think about original-brownbear@a71f1d7 ?

Just fix this by composition and never have any unnecessary if(kwargs) branching in the hot path? Just an alternative suggestion :)

[enebo]

@original-brownbear Have you examined how that works in practice in JITWatch (since you mentioed it in other PRs)? It will eliminate the conditional but then beyond that worst case it becomes a bimorphic site (which will sort of turn back into a conditional) but where it ends up inlining it will go away...although the same will happen in the conditional case if call inlines back to another callsite. I guess another thing I wonder is if it slows warmup. I guess I doubt it since this code is immediately hot.

@headius what do you think?

[original-brownbear]

@enebo you're right ... it looked smarter than it was :)

The upstream effects are the same, but the bi-morphic site version is 488 bytes vs. 424 bytes in this PR. => I take back my alternative suggestion :)

Just for reference:

Bimorphic site

Static + Conditional

[enebo]

@original-brownbear weirdly the more you program like C the more the JVM is predictable. That has gotten better over time but it is still true.