Skip to content
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

Draft: consume js inputFileSystem on the rust side #8643

Closed
wants to merge 10 commits into from

Conversation

nilptr
Copy link
Contributor

@nilptr nilptr commented Dec 8, 2024

Summary

resolves #5091

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copy link

netlify bot commented Dec 8, 2024

Deploy Preview for rspack canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit e4a3ede
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/67583fcc24fdd40008056ea8

@nilptr nilptr force-pushed the nilptr/feat/input-file-system branch from e46d8a3 to 3162187 Compare December 8, 2024 06:05
@nilptr nilptr changed the title consume js inputFileSystem on the rust side Draft: consume js inputFileSystem on the rust side Dec 8, 2024
@nilptr nilptr force-pushed the nilptr/feat/input-file-system branch from d6c9a1b to 2627707 Compare December 8, 2024 08:18
@nilptr nilptr force-pushed the nilptr/feat/input-file-system branch from 086d822 to 7a4d76e Compare December 9, 2024 15:11
@nilptr
Copy link
Contributor Author

nilptr commented Dec 9, 2024

Hi guys, I have been struggling to investigate the CI "Test Node 18" failure for 2 days. I feel I really need your help.

I believe my implementation can pass all the tests if the tests are run separately.
However, the tests will always be stuck in the config/builtin-swc-loader/swc-plugin case, when they are run concurrently.

My findings so far are that it's stuck in


even if the output is the same.

@hardfist
Copy link
Contributor

hardfist commented Dec 10, 2024

thanks it maybe related to performance regression, block_on fs binding will cause huge performance regression

I think the biggest blocker now is rspack_resolver doesn't use async_fs, which cause it has to block_on on fs binging call, which will introduce huge performance regression web-infra-dev/rspack-resolver#34.

but we can still merge this pr first if we don't enable input_fs binding by default and when we solve the rspack_resolver async_fs problem, we can enable inputfs binding

@nilptr
Copy link
Contributor Author

nilptr commented Dec 10, 2024

yeah, I also suspect the performance regression, but cannot prove it.

I'm afraid we cannot merge this PR now, because compiler.inputFileSystem has been set by default.

function createCompiler(userOptions: RspackOptions): Compiler {
const options = getNormalizedRspackOptions(userOptions);
applyRspackOptionsBaseDefaults(options);
assert(!isNil(options.context));
const compiler = new Compiler(options.context, options);
new NodeEnvironmentPlugin({
infrastructureLogging: options.infrastructureLogging
}).apply(compiler);

and

const inputFileSystem: InputFileSystem = new CachedInputFileSystem(
fs,
60000
);
compiler.inputFileSystem = inputFileSystem;

if you feel the refactors in this pr are needed, I can create a new pr and cherry pick the refactor commits.

@hardfist
Copy link
Contributor

hardfist commented Dec 10, 2024

yeah, I also suspect the performance regression, but cannot prove it.

I actually tried it before and revert cause huge performance regression introduced, you can test it in project with large modules like https://github.com/hardfist/performance-compare-ng/tree/master/apps/10000

@hardfist
Copy link
Contributor

hardfist commented Dec 10, 2024

I'm afraid we cannot merge this PR now, because compiler.inputFileSystem has been set by default.

that's easy, you can just ignore the inputFileSystem in the rust side, it's actually current's behavior

@nilptr nilptr mentioned this pull request Dec 10, 2024
2 tasks
@nilptr
Copy link
Contributor Author

nilptr commented Dec 10, 2024

I disabled inputFileSystem on the js side in this PR.

I also created another branch and cherry-picked the refactor commits. maybe #8654 is exactly what you expect.

@hardfist
Copy link
Contributor

I disabled inputFileSystem on the js side in this PR.

I also created another branch and cherry-picked the refactor commits. maybe #8654 is exactly what you expect.

yes, good job!

@nilptr nilptr closed this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to resolve xxx in project root error occurred while using inputFileSystem
2 participants