C#: Speedup Implements.qll#1011
Conversation
calumgrant
left a comment
calumgrant
left a comment
There was a problem hiding this comment.
Overall, looks good, but have a few general questions.
| j = -1 | ||
| or | ||
| result = getACompatibleInterfaceIndexer0(i, j - 1) and | ||
| convIdentity(i.getParameter(j).getType(), result.getParameter(j).getType()) |
There was a problem hiding this comment.
Because indexers cannot have type arguments.
|
|
||
| private Indexer getACompatibleInterfaceIndexer0(Indexer i, int j) { | ||
| result = getACompatibleInterfaceIndexerCandidate(i) and | ||
| convIdentity(i.getType(), result.getType()) and |
There was a problem hiding this comment.
Because indexers cannot have type arguments.
| } | ||
|
|
||
| private Method getACompatibleInterfaceMethod(Method m) { | ||
| result = getACompatibleInterfaceMethod0(m, m.getNumberOfParameters()) |
There was a problem hiding this comment.
m.getNumberOfParameters() - 1 ?
There was a problem hiding this comment.
Actually no, because getArgumentOrReturnType ranges from 0 to m.getNumberOfParameters().
| // Recursive case | ||
| ( | ||
| t1 instanceof PointerType and t2 instanceof PointerType | ||
| module Gvn { |
There was a problem hiding this comment.
If I understand correctly, this entire module is a speedup for convIdentity/2. Therefore, would it not make sense to put this module into Conversion.qll (or even better, a separate file), and to make convIdentity/2 an inline predicate that basically wraps Gvn::getGlobalValueNumber(t1) = Gvn::getGlobalValueNumber(t2). The underlying convIdentity/2 could then be made private.
There was a problem hiding this comment.
This does seem like a complicated way to achieve this speedup, given that the net gains are fairly modest apart from fixing the timeout. Is there any way of addressing the timeout more directly?
There was a problem hiding this comment.
It does a bit more than just identity conversion, it also maps method type parameters at the same index to the same value. The reason that we need this is because we need to find compatible interface implementations:
interface I
{
void M<T>(T[] x);
}
class A
{
void M<T>(T[] x) { }
}
class B : A, I { }
The overrides relation generated by the extractor does not give us that A.M() may be a target in a call to I.M(), so we need to match the name and parameter types. If we do this with ordinary equality, A.M() will not match I.M(), because the two type parameters T in I.M() and A.M() are two different entities.
|
|
||
| private newtype TGvnType = | ||
| TLeafGvnType(int i) { id(_, i) } or | ||
| TMethodTypeParameterGvnType(int i) { i = any(MethodTypeParameter p).getIndex() } or |
There was a problem hiding this comment.
I'm not 100% convinced that it's correct to project all method type params down to their index. Besides, why are method type params treated differently to type type params?
There was a problem hiding this comment.
See my comment above.
| private class LeafType extends Type { | ||
| LeafType() { | ||
| not exists(this.getAChild()) and | ||
| not this instanceof MethodTypeParameter |
There was a problem hiding this comment.
Why are MethodTypeParameter and TypeTypeParameter handled differently? You could have a constructed type having a type parameter that is a type parameter (from another unbound generic type).
There was a problem hiding this comment.
Again, see my comment above.
hvitved
force-pushed
the
csharp/implements-performance
branch
from
00c7a98 to
6135b5b
Compare
calumgrant
left a comment
calumgrant
left a comment
There was a problem hiding this comment.
Thanks for answering my questions. I agree that this really is specific to Implements and should live in this file. Great work!
3 participants
This PR addresses a timeout on
nhibernate-core; performance report here (internal link).