module: fix stat cache & simpler shebang #26266
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This simplifies the shebang function significantly. Before, it was optimized for two characters input. Any module actually parsed should however have more characters than just the shebang. The performance stays the same as before.
The current caching logic broke by [0] because it used destructuring on the module arguments. Since the exported property is a primitive counting it up or down would not have any effect anymore in the module that required that property. The original implementation would cache all stat calls caused during bootstrap. Afterwards it would clear the cache and lazy require calls during runtime would create a new cascading cache for the then loaded modules and clear the cache again. This behavior is now restored. This is difficult to test without exposing a lot of information and therfore the existing tests have been removed (as they could not detect the issue). With the broken implementation it caused each module compilation to reset the cache and therefore minimizing the effect drastically. [0] nodejs#19177
|
@BridgeAR sadly an error occured when I tried to trigger a build :( |
addaleax
approved these changes
Feb 22, 2019
Co-Authored-By: BridgeAR <ruben@bridgewater.de>
|
CI is still unavailable, have you run module benchmarks? |
jasnell
approved these changes
Feb 23, 2019
jdalton
reviewed
Feb 23, 2019
jdalton
reviewed
Feb 23, 2019
jdalton
reviewed
Feb 23, 2019
jdalton
reviewed
Feb 23, 2019
This reverts commit 512210f.
BridgeAR
added
the
author ready
BridgeAR
added a commit
to BridgeAR/node
that referenced
this pull request
Mar 1, 2019
This simplifies the shebang function significantly. Before, it was optimized for two characters input. Any module actually parsed should however have more characters than just the shebang. The performance stays the same as before. PR-URL: nodejs#26266 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR
added a commit
to BridgeAR/node
that referenced
this pull request
Mar 1, 2019
The current caching logic broke by [0] because it used destructuring on the module arguments. Since the exported property is a primitive counting it up or down would not have any effect anymore in the module that required that property. The original implementation would cache all stat calls caused during bootstrap. Afterwards it would clear the cache and lazy require calls during runtime would create a new cascading cache for the then loaded modules and clear the cache again. This behavior is now restored. This is difficult to test without exposing a lot of information and therfore the existing tests have been removed (as they could not detect the issue). With the broken implementation it caused each module compilation to reset the cache and therefore minimizing the effect drastically. [0] nodejs#19177 PR-URL: nodejs#26266 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax
pushed a commit
that referenced
this pull request
Mar 1, 2019
This simplifies the shebang function significantly. Before, it was optimized for two characters input. Any module actually parsed should however have more characters than just the shebang. The performance stays the same as before. PR-URL: #26266 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax
pushed a commit
that referenced
this pull request
Mar 1, 2019
The current caching logic broke by [0] because it used destructuring on the module arguments. Since the exported property is a primitive counting it up or down would not have any effect anymore in the module that required that property. The original implementation would cache all stat calls caused during bootstrap. Afterwards it would clear the cache and lazy require calls during runtime would create a new cascading cache for the then loaded modules and clear the cache again. This behavior is now restored. This is difficult to test without exposing a lot of information and therfore the existing tests have been removed (as they could not detect the issue). With the broken implementation it caused each module compilation to reset the cache and therefore minimizing the effect drastically. [0] #19177 PR-URL: #26266 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please have a look at the commit messages for details.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes