Skip to content
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

C#: Restrict dataflow node creation to source and source-referenced entities #17483

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Sep 16, 2024

This addresses a problem observed when libraries define a very large number of types with large class hierarchies, for example as part of generated code. Since user code is very unlikely to refer to all or even most of them, by excluding such code from dataflow node generation and therefore from the C# dataflow hooks' idea of a "relevant" type, we can make many of the type predicates defined in DataFlowPrivate.qll very much cheaper.

See discarded variant #17482 for some discussion.

@smowton smowton requested a review from a team as a code owner September 16, 2024 15:37
@smowton smowton changed the title C#: Restrict dataflow node creation to source and source-referenced entities [virtual-dispatch-inclusive bariant] C#: Restrict dataflow node creation to source and source-referenced entities [virtual-dispatch-inclusive variant] Sep 16, 2024
@github-actions github-actions bot added the C# label Sep 16, 2024
…tual dispatch targets and referenced callables (e.g., in assigning a delegate)
…ions of generic instantiations, static targets, and methods that have a body even if not flagged fromSource
@smowton smowton force-pushed the smowton/feature/csharp-dataflow-fewer-nodes-including-virtual-dispatch branch from 794a376 to 3e91f0f Compare September 17, 2024 14:02
@smowton
Copy link
Contributor Author

smowton commented Sep 17, 2024

DCA results are very positive -- no alert changes; considerably reduced cost for some projects. Started QA at https://github.com/github/codeql-qa-ops/issues/198

@smowton
Copy link
Contributor Author

smowton commented Sep 18, 2024

QA results are also very positive: positive time changes (a small number of large repositories exhibiting fairly large improvements, similar to DCA), and no alert differences (a small number of repos showing some changes in quality queries, but none reproducible locally, indicating these are cases of nondeterministic database creation).

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great work, nice to see how this significantly improves performance on some projects. I only have two minor comments.

@smowton
Copy link
Contributor Author

smowton commented Sep 19, 2024

@hvitved changes applied; change note added.

@smowton
Copy link
Contributor Author

smowton commented Sep 19, 2024

Confirmed that for the original problem database that sparked this, the predicates taking >=1s are:

Most expensive predicates for completed query XSS.ql:
        time  | evals |   max @ iter | predicate
        ------|-------|--------------|----------
        42.6s |       |              | Implements::getAnImplementedInterfaceMemberForSubType/2#8542025f_120#join_rhs@b020baas
        23.1s |    39 |  6.2s @ 3    | Implements::getAnImplementedInterfaceMemberForSubType/2#8542025f@6c8e02no
        18.1s |    21 |  9.9s @ 9    | Conversion::convRefTypeRefType/2#3840d258#reorder_1_0@627975wt
        15.9s |       |              | Member::Overridable.getAnUltimateImplementee/0#58db54c4@874a91rj
          14s |       |              | OverridableCallable::OverridableCallable.getInherited/1#dispred#78b84fce@ef4e925t
        12.6s |       |              | Member::Overridable.getAnUltimateImplementee/0#58db54c4@62ab75cc
         7.3s |       |              | Unification::Unification::Cached::typeConstraintSubsumes/2#ad05ac89@c5f84549
         6.9s |       |              | Unification::Unification::Cached::typeConstraintUnifiable/2#1c317d19@c3c4f8oo
         5.1s |       |              | __Location::Location.hasLocationInfo/5#dispred#45331897__Element::Element.getALocation/0#dispred#0f2__#join_rhs@be6b8dnq
         4.7s |    34 | 872ms @ 13   | #Type::ValueOrRefType.getABaseType/0#dispred#4af8c7e0Plus#bf@8af016sk
         4.5s |    34 | 643ms @ 8    | #Type::ValueOrRefType.getABaseType/0#dispred#4af8c7e0Plus#bf@f26df4uu
         4.1s |    15 |  3.2s @ 2    | Implements::getACompatibleRelevantInterfaceMember0/2#dffa6396#bff@fddef8s8
           4s |       |              | Member::Overridable.getAnUltimateImplementee/0#58db54c4@9a47c2q5
         3.7s |       |              | ExternalFlow::parameterQualifiedTypeNamesToString/1#c0c070ba@702c056v
         3.5s |       |              | DataFlowPrivate::commonSubTypeGeneral/2#55d9b3a5@7a7bd5u1
         3.3s |    35 | 306ms @ 12   | OverridableCallable::OverridableCallable.getInherited0/1#4f8a07e6#ffb@cad181ec
         2.6s |       |              | Implements::getARelevantInterfaceMemberCandidate/1#f08a099b@9c0f5bas
         2.5s |       |              | _AspNetCore::MicrosoftAspNetCoreMvcControllerBaseClass#82d5f012_Element::NamedElement.getName/0#disp__#higher_order_body@6cd67cqa
         2.3s |    20 |  1.6s @ 10   | Conversion::convRefTypeNonNull/2#80d5061d@627979wt
         2.1s |       |              | Implements::getAPossibleImplementor/1#9c7b7cfd@dfb416ri
         1.9s |    35 | 204ms @ 8    | _@virtualizable_Member::Modifiable.hasModifier/1#dispred#72531dc6_Member::Overridable.getExplicitlyI__#antijoin_rhs@L0#0b5f3
         1.9s |       |              | Implements::getAPotentialRelevantInterfaceMemberAux/4#bd1fda6f_1230#join_rhs@d568985e
         1.8s |    69 | 229ms @ 27   | OverridableCallable::OverridableCallable.isDeclaringSubType/1#c77dd2ec@3fb43wev
         1.8s |       |              | OverridableCallable::OverridableCallable.getInherited1/1#dispred#5fc48a55#ffb_120#join_rhs@2501a0b8
         1.8s |    15 | 987ms @ 6    | __ExternalFlow::QN::getTypeArgumentsQualifiedName/2#376a6b72#prev_ExternalFlow::QN::getTypeArguments__#join_rhs@L2#5be36
         1.8s |    15 | 994ms @ 3    | __Generics::ConstructedGeneric.getTypeArgument/1#dispred#3e84d95d_Generics::ConstructedGeneric.getTy__#join_rhs@L2#647b8
         1.6s |    17 |  1.3s @ 3    | __Unification::Gvn::ConstructedGvnTypeList.toStringConstructedPart/2#9ffaf943#prev_Unification::Gvn:__#join_rhs@L6#7e566
         1.4s |       |              | OverridableCallable::OverridableCallable.getInherited1/1#dispred#5fc48a55#ffb@1506b53o
         1.3s |       |              | ServiceStack::ServiceClass#3ba1ecfc@7a99465f
         1.2s |       |              | OverridableCallable::OverridableCallable.isDeclaringSubType/2#87f9be27_021#join_rhs@0ea8647v
         1.2s |       |              | Conversion::implicitConversion/2#bed063da@9bd65fet
         1.1s |       |              | Conversion::implicitConversion/2#bed063da@1e997f2t
           1s |       |              | ExternalFlow::parameterQualifiedTypeNamesToString/1#c0c070ba@4f5434p0
           1s |       |              | __Attribute::Attribute.getType/0#dispred#0bee65bf#bf_Element::NamedElement.getName/0#dispred#6665e8e__#antijoin_rhs@664fcdft
           1s |    15 | 636ms @ 3    | __Generics::ConstructedGeneric.getTypeArgument/1#dispred#3e84d95d_Generics::ConstructedGeneric.getTy__#join_rhs@L2#8ea80

Note DataFlowPrivate::commonSubTypeGeneral at 3.5s

@smowton smowton changed the title C#: Restrict dataflow node creation to source and source-referenced entities [virtual-dispatch-inclusive variant] C#: Restrict dataflow node creation to source and source-referenced entities Sep 19, 2024
@smowton smowton merged commit 0deefad into github:main Sep 19, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants