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

Use of external verified VecMap crate #39

Merged
merged 4 commits into from
Jan 12, 2023
Merged

Use of external verified VecMap crate #39

merged 4 commits into from
Jan 12, 2023

Conversation

jhaye
Copy link
Collaborator

@jhaye jhaye commented Jan 5, 2023

This does compile and should work out of the box.

But I want to make some performance testing first, before merging, since I replace every expression that uses closures.

@lhstrh
Copy link
Member

lhstrh commented Jan 5, 2023

@petervdonovan here is another use case for continuous benchmarking.

@jhaye
Copy link
Collaborator Author

jhaye commented Jan 6, 2023

Right, so there is basically no performance difference:
image

There are some detectable differences, but they even out overall. And they are seemingly just a result of already existing benchmark performance variation.

@jhaye jhaye marked this pull request as ready for review January 6, 2023 22:48
@jhaye jhaye requested a review from oowekyala January 12, 2023 13:10
Copy link
Collaborator

@oowekyala oowekyala left a comment

Choose a reason for hiding this comment

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

Let's merge this.

For future reference, I think putting the crate on crates.io is not a good idea as the crate is specifically tailored for the scheduler. There are other crates which implement similar functionality but with a higher level api, that wouldn't support some of the optimizations we do (like the Keyref thingy), like ordered-vecmap. Putting it on crates.io would increase the maintenance burden for something that is supposed to be internal.

I also am concerned about splitting the codebase in two. I thought until now that this pr was not targeting main and was something temporary for your fork @jhaye. I think maybe a good way to arrange this in the future is to move the vecmap crate into this repo using a cargo workspace.

@oowekyala oowekyala merged commit 9051c95 into main Jan 12, 2023
@oowekyala oowekyala deleted the vecmap branch January 13, 2023 12:17
@lhstrh lhstrh changed the title Use external verified VecMap crate Use of external verified VecMap crate Feb 23, 2023
@lhstrh lhstrh added the enhancement New feature or request label Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants