-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
src: port initializeImportMeta
to native
#57286
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
Review requested:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57286 +/- ##
==========================================
- Coverage 90.28% 90.26% -0.02%
==========================================
Files 630 630
Lines 186158 186208 +50
Branches 36484 36488 +4
==========================================
+ Hits 168067 168077 +10
- Misses 10974 10987 +13
- Partials 7117 7144 +27
🚀 New features to boost your workflow:
|
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'm not great with cpp, but it looks fine AFAICT, and conceptually checks out 🙂
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 think the original implementation is much easier to understand and maintain.
Do you have any suggestions on how to improve it? |
Not with it all being in C++. The original was plain JavaScript that only required JavaScript domain knowledge. This PR shifts it all in to C++, thus requiring the reader to know that language along with all of the underlying APIs used to implement the feature. |
CI: https://ci.nodejs.org/job/node-test-pull-request/65676/ Results
|
Banchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1680/ Results
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1682/ Results
It's not any better, reverting |
src/node_modules.cc
Outdated
@@ -579,6 +579,86 @@ void GetCompileCacheEntry(const FunctionCallbackInfo<Value>& args) { | |||
isolate, v8::Null(isolate), names.data(), values.data(), names.size())); | |||
} | |||
|
|||
static void InitImportMetaLazyGetter( | |||
Local<v8::Name> name, const v8::PropertyCallbackInfo<Value>& info) { |
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.
Nit: remove the v8::
qualifier and add v8::PropertyCallbackInfo
to the using section at the top if it is not already there.
src/node_modules.cc
Outdated
|
||
node::Utf8Value url(isolate, info.Data()); | ||
// TODO(aduh95): Find a way to avoid calling Ada twice on module that access | ||
// both `import.meta.dirname` and `import.meta.filename`. |
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.
Setting Data
to a single internal object that has both dirname
and filename
set on it the first time either is accessed on it should work, yes?
src/node_modules.cc
Outdated
target | ||
->SetLazyDataProperty( | ||
context, env->dirname_string(), InitImportMetaLazyGetter, args[1]) | ||
.Check(); |
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.
Nit: can these please avoid the use of Check()
and propagate errors up correctly?
The actual benchmarks site is asking for a login. How do we interpret this snippet? |
hmm... looking at the benchmark results there it's just not clear to me that moving the init to native code has enough realized benefit. Moving into native does make the code a bit more difficult to maintain while also being slight slower. A lazy getter defined in JavaScript could likely achieve the same result while being easier for more people to help maintain. Not going to block on it tho... just not seeing the full benefit. |
It's unclear whether a JS getter would be spec compliant, see the discussion in #57003 – that being said, I'm also a bit puzzled by the benchmark results |
What you've done here is pretty cool. But if the c++ implementation isn't faster, and we can stay spec compliant (which that discussion seemed to end that it is), I think the JS implementation would be better because it's more maintainable. |
f38430c
to
e7c7320
Compare
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1697/ Results
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1698/ Results
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1699/ Results
|
Landed in 50cfc6c |
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
PR-URL: #57286 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Edy Silva <[email protected]>
Supersedes #57003.
We should probably avoid translating the URL to a path twice when the user needs both
dirname
andfilename
, but I'm not sure if there's an elegant way to do it without crossing the C++/JS bundary.