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.

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

Tiny - Drop dep on petgraph and simplify logic for computing free pending validation #1976

Merged
merged 12 commits into from Dec 18, 2019

Conversation

@willemolding
Copy link
Contributor

willemolding commented Dec 13, 2019

PR summary

Since we were not actually using the topological sort functionality of petgraph this drops it as a dependency and uses a HashSet to calculate the leaves of the dependency graph.

// only return those that don't have anything also pending as a dependency
// as we know these will always fail
pending
.clone()

This comment has been minimized.

Copy link
@AshantiMutinta

AshantiMutinta Dec 13, 2019

Contributor

We are cloning these twice, can we get away without doing this?

This comment has been minimized.

Copy link
@maackle

maackle Dec 13, 2019

Member

Yeah I wonder if you can just get away with using addresses?

This comment has been minimized.

Copy link
@willemolding

willemolding Dec 15, 2019

Author Contributor

I rejigged it a bit. Not sure if is actually better but I don't think it is worse...

Copy link
Member

zippy left a comment

This looks good. I really like @AshantiMutinta's questions and makes sense to do them. However, at this point I'd also love us to be able to do these kinds optimizations with data perf/benchmark data that shows us that it was worth the speed up gained to make the change.

Copy link
Member

maackle left a comment

Looks good, though I also wonder if you can do it without clones, just with references. Only if it's easy.

fmt
willemolding and others added 3 commits Dec 16, 2019
@AshantiMutinta AshantiMutinta self-requested a review Dec 17, 2019
@willemolding

This comment has been minimized.

Copy link
Contributor Author

willemolding commented Dec 17, 2019

@lucksus I was unable to reproduce your issue but I'm fairly sure it was related to losing the sorted-ness of the queue. I have rewritten it to just use a filter to eliminate pending validations with pending dependencies so it should be ok.
I want to run trial to check before merging though.

@lucksus

This comment has been minimized.

Copy link
Member

lucksus commented Dec 17, 2019

@willemolding I'd suggest to merge this now and get it into 0.0.41 as it is definitely an improvement and no regression judging from the passing tests.

thedavidmeister and others added 2 commits Dec 18, 2019
@willemolding willemolding merged commit c799c65 into develop Dec 18, 2019
5 of 7 checks passed
5 of 7 checks passed
ci/circleci: app-spec-tests-sim2h CircleCI is running your tests
Details
ci/circleci: stress-tests-sim2h CircleCI is running your tests
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: cli-tests Your tests passed on CircleCI!
Details
ci/circleci: cluster-tests Your tests passed on CircleCI!
Details
ci/circleci: fmt Your tests passed on CircleCI!
Details
ci/circleci: wasm-conductor-tests Your tests passed on CircleCI!
Details
@zippy zippy deleted the simplify-dependent-validation-calculation branch Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.