Decide whether to use isolate or inject helper classes based on the advice classes#17815
Conversation
laurit
changed the title
SylvainJuge
left a comment
SylvainJuge
left a comment
There was a problem hiding this comment.
With this approach, we load all of the helper classes of an instrumentation module in the target CL or in the isolated classloader.
As we want to get rid of the getModuleGroup, which also relates to where helper classes are loaded, I wonder if we should instead provide a per-class indication of where the helper class should be loaded instead.
| } | ||
|
|
||
| @Nullable | ||
| public Boolean useIsolatedAdvice( |
There was a problem hiding this comment.
I would suggest to rename this method to reflect the fact that this is an heuristic where we infer from the advice classes that inline=false is indy and inline=true is inline.
There was a problem hiding this comment.
At least in the public api and docs we'll need to replace indy with something else, I think we could use isolated.
his is an heuristic where we infer from the advice classes that inline=false is indy and inline=true is inline.
Added a bit of javadoc to the class. Is that enough?
| * This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
| * any time. | ||
| */ | ||
| enum HelperClassStrategy { |
There was a problem hiding this comment.
do you think we could reduce to having only 2 values here ?
- injected (default) for the current behavior with inline=true, where the helper classes are just injected into the target CL
- isolated (opt-in) for when we want to isolate helper classes.
There was a problem hiding this comment.
instead of DEFAULT we could return null, is this what you meant?
There was a problem hiding this comment.
No, I was thinking there are only 2 possible states for the "outcome" of this method return value:
- using isolated classloader for helper classes
- using injected helper classes
I did not realize that the third option does in fact help with the migration, in particular for extensions, as it allows to opt-in though configuration and then find any potential issues before having to make changes to the extensions.
Maybe it's a detail, but do you think we should explicitly implement this experimental method for all the internal instrumentation or should we rely on muzzle ?
ISOLATEDfor the vast majority of helper classes (at least for the modules that do not require a shared classloader)INJECTEDfor the modules that do require a shared classloader
There was a problem hiding this comment.
I wasn't planning to implement it, but rely on auto-detection. We could try to make some kind of test that verifies that this is working as expected. I think we have one or two module (internal class loader) where auto-detection could choose injected. The class loader module is an outlier as it only works with isolated by really behaving as injected. Could consider leaving it as injected or alternatively making it work with isolated (might not be easy).
There was a problem hiding this comment.
I think that leaving class loader module as injected would definitely be fine, as it's definitely an outlier.
The main concern in this PR is to allow using the new |
When switching to indy instrumentation we need some way to tell whether an instrumenation is ready for indy or not. Currently we default to always using inline instrumenation and with
-PtestIndy=truewe switch all instrumentations to indy. We need some sort of middle ground where everything that we bundle runs with indy but user extensions could still run with inline if they are not ready for indy.This PR introduce experimental api for instumentation modules to choose whether they want to inject helper classes to the application class loader or load them into separate class loader. Implementing this api is optional, in muzzle plugin we'll inspect the advice used by the instrumentation and guess based on the advice what the instrumentation module should use. Finally we'll fall back to applying the same heuristics used by muzzle during runtime.