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.

Sign up for GitHub

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

Replace O(N) lookup with O(1) for task outputs (master) #3351

Merged
merged 2 commits into from
Sep 7, 2019
Merged

Replace O(N) lookup with O(1) for task outputs (master) #3351

merged 2 commits into from
Sep 7, 2019

Conversation

matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented Sep 6, 2019 •
edited
Loading

These changes close #3255. Supersedes #3268.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@matthewrmshin
Replace O(N) lookup with O(1) for task outputs.
5a1aaf4 
This speeds up suite validation significantly for tasks with
large numbers of outputs.  The list used required an O(N)
check for membership which, when combined with being run for
each output resulted in O(N**2) complexity.

Using the builtin set should bypass any deprecated libraries in
Python 2.

Closes cylc#3255
@matthewrmshin matthewrmshin added this to the cylc-8.0a1 milestone Sep 6, 2019
@matthewrmshin matthewrmshin self-assigned this Sep 6, 2019
@matthewrmshin matthewrmshin mentioned this pull request Sep 6, 2019
Closed
6 tasks
@oliver-sanders
Copy link
Member

Can you remove 746931d from this PR please (shouldn't go onto master)

@kinow
Copy link
Member

kinow commented Sep 6, 2019

Travis failures

./tests/authentication/05-full-control.t                        (Wstat: 0 Tests: 12 Failed: 3)
  Failed tests:  10-12
./tests/reload/17-graphing-change.t                             (Wstat: 0 Tests: 13 Failed: 1)
  Failed test:  13

@matthewrmshin
Copy link
Contributor Author

@oliver-sanders and @kinow I'll look into those soon.

@matthewrmshin
Copy link
Contributor Author

Removed 746931d and squashed the CHANGES.md changes into one.

@matthewrmshin
Copy link
Contributor Author

Test now good.

hjoliver
hjoliver approved these changes Sep 7, 2019
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

👍

@hjoliver hjoliver merged commit d54d878 into cylc:master Sep 7, 2019
@matthewrmshin matthewrmshin deleted the list-to-set branch September 9, 2019 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinow kinow kinow approved these changes

@hjoliver hjoliver hjoliver approved these changes

Assignees

@trwhitcomb trwhitcomb

@matthewrmshin matthewrmshin

Labels
None yet
Projects
None yet
Milestone
cylc-8.0a1
Development

Successfully merging this pull request may close these issues.

Speed up dependency checks for large numbers of outputs
5 participants
@matthewrmshin @oliver-sanders @kinow @hjoliver @trwhitcomb