Fix: Detect Electron version at runtime #546
Conversation
src/incoming_msg.cc
Outdated
| does include an overhead in having to call a finalizer when the | ||
| buffer is GC'ed. For very small messages it is faster to copy. */ | ||
| moved = true; | ||
| if (!hasElectronMemoryCage(env)) { |
There was a problem hiding this comment.
| if (!hasElectronMemoryCage(env)) { | |
| if (noElectronMemoryCage) { |
|
|
||
| namespace zmq { | ||
| bool hasRun = false; | ||
| bool hasElectronMemoryCageCache = false; |
There was a problem hiding this comment.
I think using the static keyword in the call site should be enough. Extra caching is not probably needed.
src/incoming_msg.cc
Outdated
| again in any case. This should not happen of course. */ | ||
| ErrnoException(env, EINVAL).ThrowAsJavaScriptException(); | ||
| return env.Undefined(); | ||
| if (!hasElectronMemoryCage(env)) { |
There was a problem hiding this comment.
It might be faster to make this variable static so that it is calculated only once.
| if (!hasElectronMemoryCage(env)) { | |
| static const auto noElectronMemoryCage = !hasElectronMemoryCage(env); | |
| if (noElectronMemoryCage) { |
There was a problem hiding this comment.
I am actually not sure, because, how I understand it, the class/function is being constructed and destroyed for each incoming message. So for each message then the function is called and re-evaluated.
That's why I opted for caching in the function one level up...
What's your thought about this?
There was a problem hiding this comment.
Yeah, it makes sense to reuse the cache for all the messages. What about making the result of the check a static property of IncomingMsg?
There was a problem hiding this comment.
Yes, good idea! I can make the changes on Thursday morning!
There was a problem hiding this comment.
It can be a separate PR. This should work for now.
There was a problem hiding this comment.
I think I have it, testing as we speak. Will commit in this branch.
There was a problem hiding this comment.
Wasn't able to make it a class member (at this moment), as I need the passed on env, which is only passed in the IntoBuffer function. At least one function call is reduced.
This way, no separate build for NodeJS and Electron v21(+) is needed. Cache the result once, to not have to look up the value on each incoming message.
Bartel-C8
force-pushed
the
runtimeElectronCheck
branch
from
9de2a11
to
2aeeb35
Compare
This way, no separate build for NodeJS and Electron v21(+) is needed.
Cache the result once, to not have to look up the value on each incoming message.