vm: refactor vm module #11392
vm: refactor vm module #11392
Conversation
|
Somewhat related: perhaps we should move |
|
ok, moved module.exports to the end! |
|
Standard location for For good reason, IMO, when looking at a module, what it exports is among the most important information. |
|
@sam-github IMHO having it at the bottom avoids any potential issues that could arise if exporting values assigned to variables for example. |
|
I wouldn't call it "standard" by any stretch. Our style is a bit all over the map. Sometimes we use the |
|
ok not "standard", but "majority"? I personally quite dislike the "exports at bottom" style, what a module exports is the most important thing about it, it should be at the top, and js has good support for this because of its scoping rules, but I'll live with any consistent style. Given how common My bigger concern is the code churn and how it affects backportability, but if we can backport the refactors, I guess we'll be OK. |
|
@sam-github It can incur a performance penalty due to the addition of hidden classes every time a new property is added, but hidden classes are still faster than dictionary mode. There is a way to force an object back into "fast" mode IIRC, but it's hacky and it's better to just define the properties in a literal from the get-go. |
Switch to the more efficient module.exports = {} pattern.
|
Updated to resolve some CI failures... @sam-github, here are the results of three benchmark runs after the refactor comparing to 7.5.0. At the results show, there is a modest but real performance improvement. |
jasnell
added
the
semver-major
|
Marking as semver-major because this does cause changes to the stack trace output |
|
Is the stack trace part of the API? I thought it was just the first line, the error message? If it can't be backported, vm maintainers will have some pain, but I'll let them speak for themselves on that. |
|
I’m pretty sure we have (I have?) treated change that only affect the content of stack traces as semver-patch in the past. |
|
If it doesn't have to be semver-major then ++ :-) |
jasnell
removed
the
semver-major
|
Pulling the semver-major label back off. |
Switch to the more efficient module.exports = {} pattern.
PR-URL: #11392
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
|
Landed in 804bb22 |
Switch to the more efficient module.exports = {} pattern.
PR-URL: #11392
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
|
needs a backport to land on v4 |
Switch to the more efficient module.exports = {} pattern.
PR-URL: #11392
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Switch to the more efficient module.exports = {} pattern.
PR-URL: #11392
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Switch to the more efficient module.exports = {} pattern.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
vm