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

Feature for Windows logical processors greater than 64 #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sectorzero
Copy link

#81

This is my first real rust program :) Started learning about a month back to learn. I used your library to write some toy programs and thought it was good opportunity to contribute to some open issue. Had good learning along the way. Please suggest any refinements/changes.

Since this can have regressions as it is hard to test on actual machines with large number of cores, I have made this a conditional compile feature. I personally did not test on machines with larger number of cores, but followed the Windows API documentation.

Also this method has been tested successfully according to this thread : https://stackoverflow.com/a/52716113

@seanmonstar
Copy link
Owner

Wow thanks for taking a stab! I'll probably be able to take a better look on Monday.

@seanmonstar
Copy link
Owner

This looks really thorough! My first question: why did you decide to make the conditional on an extended cargo feature flags? Is there any reason this shouldn't be used if available?

@sectorzero
Copy link
Author

Thanks for the review :)

Although I think this method can be used uniformly for all cases, my hunch was that we did not have enough coverage/testing in the real world yet and did not want to force a regression to those people who might be using in production and had nasty experience if something failed. How can I introduce this change without impact - is my intention. There are couple of options - either provide a boolean parameter option to the function to use this method, conditionally compile etc. I just chose one to start off with and see what you or anybody else thinks. I am looking for direction on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants