Skip to content

ExponentialBackTracking: Speedup concretise #8589

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

Merged
merged 6 commits into from
Apr 3, 2022

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Mar 29, 2022

No description provided.

@hvitved hvitved force-pushed the regex/speedup-concretise branch from cf7632c to 596b8f5 Compare March 29, 2022 09:01
@hvitved hvitved marked this pull request as ready for review March 30, 2022 13:21
@hvitved hvitved requested review from a team as code owners March 30, 2022 13:21
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Mar 30, 2022
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

You've achieved the same thing as I did in #8522, but much more concisely 👏

Could SuperlinearBackTracking.qll get the same treatment?

erik-krogh
erik-krogh previously approved these changes Mar 30, 2022
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

👍 (Assuming the new evaluations work out)

@hvitved
Copy link
Contributor Author

hvitved commented Mar 31, 2022

There was some slowdown on facebookarchive__nuclide. The latest commit tries to reduce the slowdown.

@erik-krogh
Copy link
Contributor

I think you need a merge with main to include #8532 (I think that's why Ruby is failing).

@hvitved
Copy link
Contributor Author

hvitved commented Mar 31, 2022

I think you need a merge with main to include #8532 (I think that's why Ruby is failing).

Turns out that this was already broken on main: #8624.

@hvitved hvitved force-pushed the regex/speedup-concretise branch from 3ada2c8 to 46d69cf Compare March 31, 2022 10:52
@hvitved
Copy link
Contributor Author

hvitved commented Mar 31, 2022

Rebased onto latest main without changes.

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

👍 (assuming a new evaluation works out).

@hvitved
Copy link
Contributor Author

hvitved commented Apr 3, 2022

There is still a bit of a slowdown on facebookarchive__nuclide, but less than before. I would argue that it is an evaluator bug that strictconcat is not faster than the old recursive computation (I have made @alexet aware). @erik-krogh is it acceptable to merge?

@erik-krogh
Copy link
Contributor

I would argue that it is an evaluator bug that strictconcat is not faster than the old recursive computation.

I read your comments about that, and I agree. It sounds like there are some unnecessary sorting when language[monotonicAggregates] is enabled.

@erik-krogh is it acceptable to merge?

Your solution is faster and more elegant than the solution I was planning, it massively improves the worst-case performance, and if the evaluator is fixed then the performance should be better in all cases.
So yeah, big 👍 from me.

@hvitved hvitved merged commit 50dc382 into github:main Apr 3, 2022
@hvitved hvitved deleted the regex/speedup-concretise branch April 3, 2022 15:56
@hvitved hvitved mentioned this pull request Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note Python Ruby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants