-
-
Notifications
You must be signed in to change notification settings - Fork 59
Add caching for function and method ffi_cifs #795
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
Conversation
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
src/NativeScript/Calling/FFICache.h
Outdated
|
||
for(size_t i = 0; i < signature.size(); i++) { | ||
seed <<= 4; | ||
seed |= signature[i]->type; |
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'd recommend using the Fowler–Noll–Vo hash function. This one seems to not be distributive enough.
0d7e2a3
to
7f8ee77
Compare
src/NativeScript/Calling/FFICall.cpp
Outdated
std::unordered_map<std::vector<const ffi_type*>, std::shared_ptr<ffi_cif>, SignatureHash>::const_iterator it = FFICache::global()->cifCache.find(this->signatureVector); | ||
|
||
if (it == FFICache::global()->cifCache.end()) { | ||
std::shared_ptr<ffi_cif> shared(new ffi_cif, deleteCif); |
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 find it a bit confusing that a method called checkForExistingCif
actually creates a new cif
in case there is no existing one. I suggest renaming the method (e.g. getCif
or something) or moving the cif
creation logic outside the method.
However, this is more or less a subjective statement, so if you don't agree, I am perfectly fine leaving it as is.
src/NativeScript/Calling/FFICache.h
Outdated
class FFICache { | ||
|
||
public: | ||
std::unordered_map<std::vector<const ffi_type*>, std::shared_ptr<ffi_cif>, SignatureHash> cifCache; |
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 suggest you declare a new typedef in order to avoid repeating the whole std::unordered_map<std::vector<const ffi_type*>, std::shared_ptr<ffi_cif>, SignatureHash>
everywhere you need it.
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.
👍 Nice work!
As unifying ffi signatures in metadata-generator would lead to insufficient ffi metadata in ios-runtime only ffi_cif caching in FFICall initialization is implemented.