Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP : Add env var to allow OSR at new opcodes #7609

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vijaysun-omr
Copy link
Contributor

@vijaysun-omr vijaysun-omr commented Jan 20, 2025

Add env var to allow OSR at new (new, newarray, anewarray, multianewarray and newvalue) opcodes as an experiment.

@vijaysun-omr
Copy link
Contributor Author

This is WIP currently because I have not understood getOSRInductionOffset usage to be sure that the value I returned for "news" is correct. In any case, it should not matter for doing an experiment with these "new" considered as OSR points on the JIT side (i.e. without VM support) since the VM will anyway disallow OSR at these points (as per the current default behavior) and so I doubt the value returned by getOSRInductionOffset will matter. I will make sure of this when I have time later today.

@vijaysun-omr
Copy link
Contributor Author

vijaysun-omr commented Jan 21, 2025

@gacholio do you know the history behind the values returned by getOSRInductionOffset ? e.g. we return 3 for a call.

if (osrNode->getOpCode().isCall())

As the comment/description above the routine says, it has something to do with the "destination of the OSR transition" which I thought I would check with you on, since that is expected to be decided by the VM. We take the return value from this routine and add it to the bytecode index of the OSR point (IL node) and that is what will be encoded in the JIT OSR metadata for that OSR point.

fyi @tajila

@jdmpapin
Copy link
Contributor

getOSRInductionOffset() should be 3 for new, since new is a 3-byte bytecode instruction, and if we needed to transition, we would do so after completing the allocation, in which case the first instruction to run in the interpreter should be the one after new. Not all of the allocation bytecode instructions are the same size though, e.g. newarray is 2 bytes and multianewarray is 4 bytes

I'm not sure that the incorrect offset 0 wouldn't affect a performance experiment. In particular, I think it might prevent us from generating a pending push store for the result of the allocation

It's true that we could use something like this to measure performance even if OSR transitions after allocations aren't functionally correct, but only if the benchmark never causes OSR guards to be patched. If we ever invalidate an OSR assumption, then we could attempt to transition at an allocation even if the allocation never causes invalidation itself. For example, consider this sequence:

invokestatic Foo.foo()V     // not inlined (invalidation point)
new Bar                     // experimentally assumed to be an invalidation point
                            // OSR guard will be inserted here
dup
invokespecial Bar.<init>()V // inlined with HCR guard (fear point)

We would transition at the OSR guard after new if invalidation happened during the call to foo. (Depending on what precedes this, we could potentially also transition there if invalidation occurred even earlier.)

@vijaysun-omr
Copy link
Contributor Author

Thanks for the research, Devin, I will make the change for the offset used when I get a chance, and let you know.

@gacholio
Copy link
Contributor

gacholio commented Feb 3, 2025

do you know the history behind the values returned by getOSRInductionOffset

Has this been explained? I personally don't know anything about the JIT OSR internals.

@vijaysun-omr
Copy link
Contributor Author

I updated the getOSRInductionOffset code based on Devin's comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants