-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
lib: modify NativeModule to use compileFunction #23837
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
dc9f7e1
efc0892
19262a0
9ec01a2
382e3e8
72d542b
690fe8b
ab4d9ad
d2dcd54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,7 +109,6 @@ | |
|
||
// Create this WeakMap in js-land because V8 has no C++ API for WeakMap | ||
internalBinding('module_wrap').callbackMap = new WeakMap(); | ||
const { ContextifyScript } = internalBinding('contextify'); | ||
|
||
// Set up NativeModule | ||
function NativeModule(id) { | ||
|
@@ -120,7 +119,6 @@ | |
this.exportKeys = undefined; | ||
this.loaded = false; | ||
this.loading = false; | ||
this.script = null; // The ContextifyScript of the module | ||
} | ||
|
||
NativeModule._source = getInternalBinding('natives'); | ||
|
@@ -220,15 +218,6 @@ | |
return NativeModule._source[id]; | ||
}; | ||
|
||
NativeModule.wrap = function(script) { | ||
return NativeModule.wrapper[0] + script + NativeModule.wrapper[1]; | ||
}; | ||
|
||
NativeModule.wrapper = [ | ||
'(function (exports, require, module, process, internalBinding) {', | ||
'\n});' | ||
]; | ||
|
||
const getOwn = (target, property, receiver) => { | ||
return ReflectApply(ObjectHasOwnProperty, target, [property]) ? | ||
ReflectGet(target, property, receiver) : | ||
|
@@ -237,8 +226,7 @@ | |
|
||
NativeModule.prototype.compile = function() { | ||
const id = this.id; | ||
let source = NativeModule.getSource(id); | ||
source = NativeModule.wrap(source); | ||
const source = NativeModule.getSource(id); | ||
|
||
this.loading = true; | ||
|
||
|
@@ -270,29 +258,29 @@ | |
const cache = codeCacheHash[id] && | ||
(codeCacheHash[id] === sourceHash[id]) ? codeCache[id] : undefined; | ||
|
||
ryzokuken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// (code, filename, lineOffset, columnOffset | ||
// cachedData, produceCachedData, parsingContext) | ||
const script = new ContextifyScript( | ||
source, this.filename, 0, 0, | ||
cache, false, undefined | ||
const fn = internalBinding('contextify').compileFunction( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there any reason to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Umm, it's only called once? Had it been called multiple times, I would have refactored it into a single destructure statement up top. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its called every time we load an internal module, and we have a great many of those. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @devsnek Oh wait, I get it now, sorry. Will change this. |
||
source, | ||
this.filename, | ||
0, | ||
0, | ||
cache, | ||
false, | ||
undefined, | ||
[], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ☝️ I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jdalton I used an empty array since that's the default value I used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My point was more reducing throw away object creation. |
||
['exports', 'require', 'module', 'process', 'internalBinding'] | ||
ryzokuken marked this conversation as resolved.
Show resolved
Hide resolved
ryzokuken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
|
||
// This will be used to create code cache in tools/generate_code_cache.js | ||
this.script = script; | ||
|
||
// One of these conditions may be false when any of the inputs | ||
// of the `node_js2c` target in node.gyp is modified. | ||
// FIXME(joyeecheung): Figure out how to resolve the dependency issue. | ||
// When the code cache was introduced we were at a point where refactoring | ||
// node.gyp may not be worth the effort. | ||
if (!cache || script.cachedDataRejected) { | ||
ryzokuken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!cache) { | ||
compiledWithoutCache.push(this.id); | ||
} else { | ||
compiledWithCache.push(this.id); | ||
} | ||
|
||
// Arguments: timeout, displayErrors, breakOnSigint | ||
const fn = script.runInThisContext(-1, true, false); | ||
const requireFn = this.id.startsWith('internal/deps/') ? | ||
NativeModule.requireForDeps : | ||
NativeModule.require; | ||
|
Uh oh!
There was an error while loading. Please reload this page.