Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Apr 8, 2019

This appears to restore performance for DeadStoreOfProperty.ql to the one we had before #1113. #1156 also attempted to restore that performance, but that was only for the more serious dataflow stage timeouts.

                                   PRE-1113              CURRENT-master         THIS-PR
Project                            74e701f7ebd@5441352^  74e701f7ebd@662ad4b2c  74e701f7ebd@3ea4958a9
mxnet.js                           248                   921                    253

I will start a larger evaluation now.

@ghost ghost added JS WIP This is a work-in-progress, do not merge yet! labels Apr 8, 2019
@ghost ghost self-requested a review as a code owner April 8, 2019 11:16

/** Gets a data flow node defining a descriptor attribute of the property being defined. */
pragma[inline]
DataFlow::PropWrite getAPropertyAttribute() {
Copy link

Choose a reason for hiding this comment

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

Can we mark this predicate is internal? Otherwise people might start using it in unexpected ways and perhaps try to do recursion through it, which won't work anymore now that it's pragma-inlined.

Copy link
Author

@ghost ghost Apr 10, 2019

Choose a reason for hiding this comment

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

That would mean we were using an internal predicate in queries, that is the kind of thing we wanted to avoid in #1211. I have reformulated the predicate in the style of hasPropertyWrite, the optimizer is happy with that change:

(scroll to the right)

Project                            74e701f7ebd@5441352^  74e701f7ebd@662ad4b2c  74e701f7ebd@b5f92dfa7
A2Z-F15                            758                   740                    745
angular-gulp-webpack-starter       381                   403                    381
any-balance-providers              144                   141                    140
azure-sdk-for-node                 405                   420                    416
ChakraCore                         432                   435                    433
descartes                          247                   246                    250
gecko-dev                          1332                  1335                   1340
mxnet.js                           248                   921                    258  <<<<<
node                               761                   907                    762
tatami                             117                   71                     73
TypeScript                         51                    52                     53

getAPropertyAttribute was introduced after the feature freeze, so this is not a serious breaking change.

@xiemaisi
Copy link

Thanks; I like the new formulation. Let's do a full evaluation on everything, though. Partial evaluations were what got us into this situation, and we've missed the last distribution build anyway, so we can spare a few days' wait.

@ghost
Copy link
Author

ghost commented Apr 16, 2019

The full evaluation for the two queries that use the predicate in their whitelist (the predicate is not used anywhere else) show some nice speedups. The overall analysis time is reduced to a factor 0.86.

This is ready to merge.

@ghost ghost removed the WIP This is a work-in-progress, do not merge yet! label Apr 16, 2019
@xiemaisi xiemaisi merged commit 7af4baf into github:master Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant