On Fri, 31 May 2024 18:43:49 GMT, Ioi Lam <[email protected]> wrote: >> The current algorithm says: >> >> for each bytecode in each method: >> switch(bytecode) { >> case getfield: >> case outfield: >> InterpreterRuntime::resolve_get_put(bc, raw_index, mh, cp, false >> /*initialize_holder*/, CHECK); >> break; >> .... >> } >> >> What I'm proposing is: >> >> for each ResolvedFieldEntry >> bool success = InterpreterRuntime::resolve_get_put(getfield, raw_index, >> nullptr /* mh */, cp, false /*initialize_holder*/, CHECK); >> if (success) { >> // also resolve for put >> InterpreterRuntime::resolve_get_put(putfield, raw_index, nullptr /* mh >> */, cp, false /*initialize_holder*/, CHECK); >> } >> >> >> The `method` parameter is not critical as the "current" algorithm attempts >> resolution with multiple methods - once for each method that references the >> ResolvedFieldEntry. The resolution logic already has to handle dealing with >> different rules for different types of methods (ie `<clinit>` & `<init>`) >> for normal execution and "knows" not to resolve entries (like puts of field >> fields) regardless of the method as they need to do additional runtime >> checks on every access. >> >> The same will apply to invoke bytecodes later.... it feels safer to do only >> what the bytecodes in some method have asked for but the runtime already has >> to be robust against different kinds of gets/puts or invokes targeting the >> same cp entries. By eagerly resolving we're not giving up any safety. >> >> If you really want to only resolve the exact cases (ie: gets not puts, etc) >> that were resolved in the training run, then we need to write out as part of >> the classlist more explicitly what needs to be resolved: >> ie: >> >> @cp_resolved_gets 4, 7 8 >> @cp_resolved_puts 7 8 10 > > This makes sense. I will try to prototype it in the Leyden repo and then > update this PR.
I tried skipping the `methodHandle` parameter to `InterpreterRuntime::resolve_get_put` but it's more complicated than I thought. 1. The `fieldDescriptor::has_initialized_final_update()` will return true IFF the class has `putfield` bytecodes to a final field outside of `<clinit>` methods. See [here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/rewriter.cpp#L463) 2. When `InterpreterRuntime::resolve_get_put` is called for a `putfield`, it adds `putfield` to the `ResolvedFieldEntry` only if `fieldDescriptor::has_initialized_final_update()` is false. See [here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/interpreterRuntime.cpp#L703) 3. `InterpreterRuntime::resolve_get_put`calls `LinkResolver::resolve_field()`, which always checks if the `methodHandle` is `<init>` or not, without consulting `fieldDescriptor::has_initialized_final_update()`. See [here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/linkResolver.cpp#L1040) (2) is an optimization -- if a method sets final fields only inside `<clinit>` methods, we should fully resolve the `putfield` bytecodes. Otherwise every such `putfield` will result in a VM call. (3) is for correctness -- make sure that only `<init>` can modify final fields. I am pretty sure (2) and (3) are equivalent. I.e., we should check against the method in (3) only if `fieldDescriptor::has_initialized_final_update()` is true. However, (3) is security related code, so I don't want to change it inside an optimization PR like this one. Without fixing that, I cannot call `InterpreterRuntime::resolve_get_put` with a null `methodHandle`, as it will hit the assert. This goes back to my original point -- I'd rather do something stupid but correct (call the existing APIs and live with the existing behavior), rather than trying to analyze the resolution code and see if we can skip certain checks. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1624928922
