WELD-2812 Improve Invoker exception messages#3133
Conversation
|
Thank you for the PR. CC @Ladicek in case you are interested; I know you authored some of these parts. |
|
I generally agree that better exceptions are better, but the performance impact is exactly why I made the specification relatively vague on this. |
Yes, I did have some simple perf tests with Weld and invokers where I tested the initial drafts. Although the way Andrew wrote it is very nice (another method handle basically) so I assume the impact will be barely noticeable in which case it is probably just fine 🤷 |
I have tried to measure this with some JMH benchmarks that we already have in place and also tried adding some others (not commited) [benchmark repo link]. The results show mostly negligible differences - and by that I mean <1% difference so long as the method uses lookup for some of its parts or does some noticeable work. I am not sure which
That's neat idea, it would eliminate the overhead - care to give it a go?
Doesn't need to be NPE, but the exception message should probably say that |
Sorry, I mixed up my tests. I ran the test against
I'll give this a try
I'll add a message for this as a separate case. |
manovotn
changed the title
|
FTR, I've created a tracking issue for this and changed the title of this one to include issue number - https://issues.redhat.com/browse/WELD-2812 |
Azquelt
force-pushed
the
invoker-exception-messages
branch
from
ba10eed to
65d9c2b
Compare
|
I've updated this PR so that it checks the instance and arguments only if a Method handles does not allow the equivalent of multiple catch blocks on a single I've put the new changes in a new commit for comparison, but they can all be squashed before it's merged. A quick sniff test suggests that the performance is much closer to the original and quite a bit improved from the previous approach of doing the additional checks every time before the method is called. I did notice that in my original approach, I'd used |
|
Sorry for the delay, I was sick and out of office. |
manovotn
left a comment
manovotn
left a comment
There was a problem hiding this comment.
This is looking great @Azquelt!
I have done some perf testing as well and it is essentially indistinguishable from the state without validation 👍
I've added a few minor comments that I spotted while reviewing.
Feel to squash the commits into one as well.
...uillian/src/test/java/org/jboss/weld/tests/invokable/exceptions/InvokableExceptionsTest.java
Outdated
Show resolved
Hide resolved
impl/src/main/java/org/jboss/weld/invokable/InvokerValidationUtils.java
Outdated
Show resolved
Hide resolved
| MethodHandle cceCatch = MethodHandles.throwException(mh.type().returnType(), ClassCastException.class); | ||
| cceCatch = MethodHandles.collectArguments(cceCatch, 1, checkTypes); | ||
|
|
||
| mh = mh.asType(mh.type().changeParameterType(0, Object.class)); // Defer the casting the instance to its expected type until we're inside the ClassCastException try block |
There was a problem hiding this comment.
Do I understand it correctly that this is needed strictly because the adapter method handle (cceCatch) must have the exact same type as the target method handle? I am a bit surprised this doesn't cause issues when invoking the method handle for the original (user defined) method.
Also, a nitpicker note - the comment has a superflous the in it :)
There was a problem hiding this comment.
Do I understand it correctly that this is needed strictly because the adapter method handle (
cceCatch) must have the exact same type as the target method handle?
Yes, this is a requirement of MethodHandles.catchException.
I am a bit surprised this doesn't cause issues when invoking the method handle for the original (user defined) method.
I think this works because MethodHandle.invoke will attempt to adapt the arguments to the types expected by the method handle, in the same way that MethodHandle.asType would.
Without this line which changes the parameter type, the final MethodHandle will have:
- return type the same as the user's method
- first parameter type equal to the declaring class of the user's method (or
Objectfor static methods) - second parameter of
Object[]
This will cast the instance to the expected type when invoke is called, so when we're trying to catch and handle the ClassCastException ourselves, we need to ensure that the first parameter has type Object and that the cast to the expected type is done within our catch block.
Otherwise, it would also be possible satisfy the requirement that parameter types match exactly by adapting the cceCatch method handle to match the user's method:
MethodHandle cceCatch = MethodHandles.throwException(mh.type().returnType(), ClassCastException.class);
cceCatch = MethodHandles.collectArguments(cceCatch, 1, checkTypes);
cceCatch = cceCatch.asType(cceCatch.type().changeParameterType(1, mh.type().parameterType(0)));
mh = MethodHandles.catchException(mh, ClassCastException.class, cceCatch);However, this results in the invoke call doing the cast which means we can't catch the exception.
There was a problem hiding this comment.
I think this works because MethodHandle.invoke will attempt to adapt the arguments to the types expected by the method handle, in the same way that MethodHandle.asType would.
Ah, right, that's it.
I keep thinking along the lines of invokeExact which is what probably wouldn't work.
Thanks for detailed explanation.
If an invoker invocation causes a ClassCastException, NullPointerException or IllegalArgumentexception, validate the instance and arguments to see if the exception was caused by passing invalid values. If so, discard the original exception and throw a new exception with more information. Doing these checks only when the invocation throws an exception avoids the overhead of doing these checks before every successful invocation.
Azquelt
force-pushed
the
invoker-exception-messages
branch
from
65d9c2b to
5f393ee
Compare
|
I think I've addressed all the review comments and have squashed it ready to merge. |
|
Thanks for contributing :) |
3 participants
Some of the exception messages you can get from calling an Invoker with the wrong arguments are a bit obtuse and I wanted to look at providing a bit more information.
To do this, I'm doing extra checks before calling the method to ensure that:
Most of these checks have to be done before the argument array is spread to the actual arguments, since the MethodHandles implementation will throw an exception there if things are the wrong types.
If there are transformers, type checking is done against the input type of the transformer.
The null check for the instance is done after the transformers run because an instance transformer can convert a
nullto a real instance.The null check for primitive arguments, however, has to be done before the argument array is spread, since the spread operation will fail if the arguments cannot be converted to the primitive type successfully. I think this is safe because we separately check that the types for an argument transformer match the method parameters, and if that's a primitive type then the argument transformer isn't able to return
nullthere.Performance
I'm doing all of these checks before method invocation. Running an invoke call to
InvokableBean.ping10 million times took 5.2 seconds before these changes and 5.7 seconds afterwards, so there is a performance impact there which we may want to look into further.One other thought I had was that it might be possible to catch
NullPointerExceptionandClassCastExceptionand only do these checks in the case where one of those exceptions was thrown. If we find that there's a problem with the instance or arguments at that point, we can replace the exception thrown by the MethodHandles implementation. If all our checks pass we'd rethrow the original exception.Examples
Not enough arguments:
Before:
After:
Arguments array is
nullwhen arguments are requiredBefore:
After:
^I wasn't sure whether this one should still be a
NullPointerExceptionor whether we should treat it the same as when the user doesn't pass enough arguments.Primitive argument is null:
Before:
After:
Primitive argument has the wrong type:
Before:
After:
Instance has the wrong type:
Before:
After:
Null instance for a non-static method:
Before:
After: