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

Jump to bottom

New atm features rebased #10018

Merged
merged 42 commits into from Sep 23, 2022
Merged

New atm features rebased #10018

merged 42 commits into from Sep 23, 2022
+63,956 −38,346

Conversation

kaeluka
Copy link
Contributor

@kaeluka kaeluka commented Aug 11, 2022 •
edited

This PR

This takes the new-atm-features branch (but excludes a few experimental commits near its top), rebases it on top of current main, and removes the obsolete features that have been replaced.

@kaeluka kaeluka force-pushed the new-atm-features-rebased branch from b5495b4 to 2c28d24 Compare Aug 11, 2022
@kaeluka
Copy link
Contributor Author

kaeluka commented Aug 11, 2022

Before we can merge this, we need to (at least) train/evaluate/release the models, and add a commit here that makes the branch depend on the new model.

@kaeluka kaeluka requested a review from henrymercer Aug 11, 2022
@kaeluka
Copy link
Contributor Author

kaeluka commented Aug 11, 2022 •
edited

I just requested a review from @henrymercer - this is a pretty big change set. This is the branch we're aiming to release, as discussed yesterday evening.

We've been using most of it for many months now, which IMO means it'd be most important to take a look at the changes I've done today.

I've spotted one minor bug in one of the features when doing this, but I'd rather keep everything as stable as possible to minimize changes as much as possible (the bug also affects current main).

henrymercer
henrymercer reviewed Aug 12, 2022
View changes
Copy link
Contributor

@henrymercer henrymercer left a comment •
edited

A bunch of comments around the definition of the new features. No comments on the tests — those look great.

Before we can merge this, we need to (at least) train/evaluate/release the models, and add a commit here that makes the branch depend on the new model.

In addition to what you state here, we should also run the security-extended vs security-extended-plus-atm experiment to confirm that this change keeps us within our runtime performance budget.

...rimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll Outdated Show resolved Hide resolved
...rimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll Outdated Show resolved Hide resolved
...rimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll Outdated Show resolved Hide resolved
...rimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll Outdated Show resolved Hide resolved
...rimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll Outdated Show resolved Hide resolved
...rimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll Outdated Show resolved Hide resolved
...rimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll Outdated Show resolved Hide resolved
...rimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll Show resolved Hide resolved
javascript/ql/experimental/adaptivethreatmodeling/test/generic_feature_testing/EmptyFeature.ql Show resolved Hide resolved
javascript/ql/experimental/adaptivethreatmodeling/test/generic_feature_testing/test.html Outdated Show resolved Hide resolved
@kaeluka
Copy link
Contributor Author

kaeluka commented Aug 16, 2022

Experiment came back, 34% slowdown. @tiferet, is this a blocker?

@kaeluka kaeluka mentioned this pull request Aug 16, 2022
Closed
@tiferet
Copy link
Contributor

tiferet commented Aug 16, 2022

Experiment came back, 34% slowdown. @tiferet, is this a blocker?

I think that's a question for @henrymercer: is this still within the acceptable time cutoff relative to a run without boosted queries?

@henrymercer
Copy link
Contributor

henrymercer commented Aug 16, 2022

The KR is 25%. I think we should spend some time optimizing the new features to keep us within this performance budget.

@tiferet
Copy link
Contributor

tiferet commented Aug 16, 2022

Experiment came back, 34% slowdown. @tiferet, is this a blocker?

Is this a 34% slowdown relative to the unboosted version or relative to the current version of ATM?

Also, @kaeluka do you have ideas for how to optimize this? We should do that before proceeding with the new data generation, because we want to generate data from the final version of the PR exactly as we're going to merge it.

@kaeluka
Copy link
Contributor Author

kaeluka commented Aug 16, 2022

Is this a 34% slowdown relative to the unboosted version or relative to the current version of ATM?

Relative to the unboosted version :)

Also, @kaeluka do you have ideas for how to optimize this? We should do that before proceeding with the new data generation, because we want to generate data from the final version of the PR exactly as we're going to merge it.

Optimizing CodeQL performance is one area where I know next to nothing :( the best I can do is to look for "obvious" mistakes, but in my experience the obvious ones I'm able to recognize are also caught by QL-for-QL (and QL-for-QL is coming up with nothing for this PR). I'd be very eager to help out on the optimization, but I think I can't drive this.

@kaeluka kaeluka force-pushed the new-atm-features-rebased branch from 73ab279 to a82d9e2 Compare Aug 25, 2022
esbena and others added 23 commits Sep 1, 2022
@esbena @henrymercer @kaeluka
Apply suggestions from code review
57041aa 
Co-authored-by: Henry Mercer <henrymercer@github.com>
@esbena @kaeluka
address review comments
548f028
@kaeluka
add docstring examples
095beb3
@kaeluka
remove Input_ArgumentIndexAndAccessPathFromCallee
82069cf
@kaeluka
use proper import instead of inlining
d0faf71
@kaeluka
ATM: new feature to list all imports in an endpoint's file
25db666
@kaeluka
documentation for new feature
93aa279
@kaeluka
documentation for calleeImports ATM feature
3e86076
@kaeluka
add functionInterfacesInFile and surroundingFunctionParameters features
3422bde
@kaeluka
add documentations and rename a feature
f8b3c27
@kaeluka
use ? for unknown parameternames
96919ee
@kaeluka
performance fixes
67500f8
@kaeluka
fix bug in InputArgumentIndex feature
db73a62
@kaeluka
add assignedToPropName feature to let the model improve number of fal…
9468f62 
…se positives for XSS query
@kaeluka
add stringConcatenatedWith feature to help the model learn that strin…
ed75080 
…g concatenation leaves are usually not sinks
@kaeluka
remove obsolete features
3aa4e29
@kaeluka
fix ql-for-ql warnings
068a948
@kaeluka
fix now-broken tests
ac097d5
@kaeluka @henrymercer
Review comments
b9cb60c 
Co-authored-by: Henry Mercer <henrymercer@github.com>
@kaeluka
better documentation
caf3959
@kaeluka
autoformatter
f59c48e
@kaeluka
remove unused code
cdbab18
@kaeluka
optimize performance of a helper-predicate
81d02cc
@kaeluka kaeluka force-pushed the new-atm-features-rebased branch from a82d9e2 to 81d02cc Compare Sep 1, 2022
henrymercer
henrymercer approved these changes Sep 20, 2022
View changes
Copy link
Contributor

@henrymercer henrymercer left a comment

LGTM! A small point of feedback: for future PRs, particularly those with big diffs, please merge in main rather than force pushing if possible once someone's started a review. That makes it possible to just review the things that have changed when giving a follow up review. Thanks!

@tiferet tiferet marked this pull request as ready for review Sep 21, 2022
@tiferet tiferet requested a review from a team as a code owner Sep 21, 2022
@kaeluka kaeluka merged commit 33d30a0 into main Sep 23, 2022
10 checks passed
@kaeluka kaeluka deleted the new-atm-features-rebased branch Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henrymercer henrymercer

@github/codeql-ml-powered-queries-reviewers codeql-ml-powered-queries-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kaeluka @tiferet @henrymercer @esbena