Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add functionality to list connected gpu(s) #140
Add functionality to list connected gpu(s) #140
Changes from 2 commits
4a1d1d7
af6844a
e1b488b
83cde41
91e2e52
2068884
aac8a8d
ece67ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a fragile and expensive comparison. Can we instead match the classes based on their hexadecimal value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would I do that?
db.classes
appears to be a hash map ofString
toClass
I thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I wasn't aware. I'll have to read up on the documentation of this new library. It might take me some time to really digest the information and provide a decent review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look at the other library I linked originally as it may be better anyway since it bundles the
pci.ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't.
A bundled database means that it could fall out of date at any point in time and that'll affect the information we provide to the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok makes sense. Any suggestions on what I could try then since the hashmap is built using strings as keys? Would you prefer I tried making a PR for that library to change it before we use it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really mind it, but I'd be interetested to know why the author opted for the corresponding value rather than the short hexadecimal values assigned to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just used hyperfine and reading the GPU is taking quite a while.
Without showing GPU:
With:
Is this acceptable as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's reasonable and expected; it's parsing a 36000-line database after all.
We'll have to implement a caching mechanism at some point, whether that goes in libmacchina or macchina is another story, but I assume it's the client or consumer, generally speaking, that needs to cache the output and not the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright cool. So then is there anything else you would like from this PR? I can have a look at making a PR for that library to use maybe
u8
for the keys if you think that would help? Also are there any changes to documentation / testing I should make here