JavaScript: Fix InconsistentNew performance regression.
#603
Conversation
This file contains hidden or 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
added 3 commits
November 30, 2018 15:40
All the filtering is now done in `getALikelyCallee`, to which I have also added an additional parameter that improves the join in the `select` clause. I've also simplified the alert message to no longer use `toString`, which isn't meant for alert messages anyway. (This is an old query.)
ghost
approved these changes
Dec 3, 2018
|
This might not be tested with a dist-upgrade before the release, so ping @asger-semmle for a second review. |
asger-semmle
approved these changes
Dec 3, 2018
Contributor
asger-semmle
left a comment
asger-semmle
left a comment
There was a problem hiding this comment.
I gave it a brief read earlier with nothing to remark. LGTM
Author
We have one more distribution upgrade before the release (but of course more eyes are always good). |
cklin
pushed a commit
that referenced
this pull request
May 23, 2022
Add port of the existing compiler-tracing.spec files to the new Lua tracing infrastructure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
As noted by Alex ET, this query has significantly regressed in performance from 1.18 on
rhino. I can't immediately see any query or library changes that would have caused it, but it's not difficult to fix by refactoring the query; see first commit.Looking at the results on
rhino, I then noticed that the query was producing a humongous number of result links since it listed all calls to the function withnewand all without, ballooning 17 alerts onrhinointo >80K result tuples. The second commit fixes this by only showing one call withnewand one without, picking the first one when sorted lexicographically by file name, line and column.Comparing three versions of the query (before this PR, after the first commit, and after the second commit) shows no changes in alerts, and reasonable performance (internal link). Note that the table shows only the runtimes for the query itself, but I ran
definitions.qlbefore to warm the cache. While some projects exhibit a slowdown of ten seconds or more, these are outliers, and worth the improved quality of results, I think.Since this is a regression from 1.18, I'm proposing this as a hotfix.