-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: static model loader #603
Conversation
} | ||
|
||
char* buffer = data.alloc(this->len); | ||
if (buffer == nullptr) { return reinforcement_learning::error_code::static_model_load_error; } |
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.
Wondering if model_data class can use a method to encapsulate these operations. i.e. alloc + copy + set size
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.
put a set_data function in data_data to handle this
new New<FactoryContext>(() => { | ||
var vwModelArray = vwModelEnumerable.ToArray(); | ||
GCHandle handle = GCHandle.Alloc(vwModelArray, GCHandleType.Pinned); | ||
try { |
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.
Is there a way to check if this makes a copy here? Unclear to me...
CreateFactoryContextWithStaticModel does make a copy. Wondering if there are 2 copies happening.
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.
There is a copy happening here but I believe it is necessary since they pass in a List which doesn't have a way to obtain a pointer to it's internal array
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.
There is a way to avoid the copy on .NET 5 or above with List<> types, but not generally, no.
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.
Looks good to me. We can always resolve memcpy issues in later PRs after a little research.
No description provided.