-
Notifications
You must be signed in to change notification settings - Fork 34
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
Bytecode instrumentation cache #229
Conversation
Signed-off-by: Evgenii Moiseenko <[email protected]>
Signed-off-by: Evgenii Moiseenko <[email protected]>
byte[] bytes = instrument(originalName(name)); | ||
byte[] bytes = null; | ||
if (bytecodeCache != null) { | ||
bytes = bytecodeCache.get(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use putIfAbscent(..) { .. }
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the usage of computeIfAbsent
in fact makes code more verbose, because:
instrument
throws checked exceptionIOException
which has to be handled inside lambda.bytecodeCache
might be null and it is Java file, so we cannot use Kotlin's?
syntax sugar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is how the code looks like:
if (bytecodeCache != null) {
bytes = bytecodeCache.computeIfAbsent(name, className -> {
try {
return instrument(originalName(className));
} catch (IOException e) {
throw new RuntimeException(e);
}
}
);
} else {
bytes = instrument(originalName(name));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove checked exception from instrument
, but still after this there would be two similar calls to instrument
(in case when we have the cache, and when we don't)
Currently bytecode instrumentation can take significant time. Moreover, it is performed for each scenario even when testing the same data structure.
In the context of the adaptive planning feature #158, instrumentation time can interfere with the invocations time, thus making it harder to get reliable prediction for the average invocation time.
This PR provides simple instrumented bytecode caching, thus mitigating the performance overhead of instrumentation.