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
[Feature branch] JS: Migrate to shared dataflow library #14412
Open
asgerf
wants to merge
155
commits into
github:js/shared-dataflow-branch
Choose a base branch
from
asgerf:js/shared-dataflow
base: js/shared-dataflow-branch
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
[Feature branch] JS: Migrate to shared dataflow library #14412
asgerf
wants to merge
155
commits into
github:js/shared-dataflow-branch
from
asgerf:js/shared-dataflow
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9e3285e
to
e4bc193
Compare
e4bc193
to
e29aa29
Compare
1457ea5
to
a0202fd
Compare
0e5e149
to
70d675b
Compare
Type-based pruning is confused by the different tests being interleaved, so we additionally want to have a test that is independent from the other parts of this test.
This seems to fix a performance issue for RegExpInjection in angular
fce379d
to
a02ab2a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Targeting a feature branch
This PR is targeting a branch named
js/shared-dataflow-branch, notmainas we normally would.mainwill happen at a later point, and will require thatjs/shared-dataflow-branchshould be that the code has been reviewed, and is considered good enough to go intomainonce the above criteria are met.main. We don't want to have to go back a re-review the code for readability later.Review order
Commits have been organised into a reading order. The large number of commits is there to make it easier to the review.
Note that not every commit can compile in isolation. I split some commits into smaller chunks for easy of readability, but you may sometimes see odd artifacts like see a trailing
and/or, an empty predicate body, or the occasional mention of a class that doesn't exist yet.Otherwise the overarching order of commits are:
ConfigSig-style and updating testsConfigSigstyle, generic test output changesSteps across function boundaries
The most difficult change for JS is that steps across function boundaries are no longer permitted, except for jump steps which can result in FPs and performance issues.
This means the following type of steps need to be rewritten:
getALocalSource(), because that can cross function boundaries.loadStoreStepfeature, which is no longer supported.The current status of this migration is:
Array,Map,Set,Promise, and generators.loadStoreStep.String#replacehas been converted, as it is the only string-related step that needed to be ported.Related reading:
Awaitedtoken: https://github.com/github/codeql-javascript-team/issues/423Evaluation
The latest evaluation can be found here. As mentioned there are too many performance regressions at the moment, and alerts need to be triaged.