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

chore: significant speedup by skipping filing-cabinet ts-config parsing #237

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

FauxFaux
Copy link
Contributor

@FauxFaux FauxFaux commented Feb 5, 2020

madge is annoyingly slow for us. This patch takes it from 1m26s to 7s (>10x speedup), for ~2,000 typescript files.

filing-cabinet, which the dependency walking stuff runs internally, runs this incredibly slow config loader in the loop. Instead, do it at the top, and pass the object in, which it already supports.

Config loading mostly copied from: https://github.com/dependents/node-filing-cabinet/blob/74185e06edbbcbb3062265946adbada9b5584999/index.js#L199-L201

This is essentially just doing JSON.parse(fs.loadFileSync(..)), aiui. except it copes with some typescript weirdness (e.g. comments in the json).

I'm not sure that's really the best place to translate the config object, but the program.tsConfig above seems not to be hit? I don't really understand why.

@pahen
Copy link
Owner

pahen commented Feb 6, 2020

Sweet! I don't use TypeScript myself so I didn't experienced this issue. The fix looks good but I think this should probably be fixed in https://github.com/dependents/node-dependency-tree/blob/master/lib/Config.js instead to take benefit of the improvement when not using Madge too.

@pahen
Copy link
Owner

pahen commented Feb 27, 2020

Should work now if you do a fresh install of Madge @FauxFaux

1 similar comment
@pahen
Copy link
Owner

pahen commented Feb 27, 2020

Should work now if you do a fresh install of Madge @FauxFaux

@FauxFaux
Copy link
Contributor Author

FauxFaux commented Jun 7, 2020

This change, which we're still running, offers a significant speedup still; from ~21s -> ~9s. (we have more typescript now!).

I am not sure why.

@FauxFaux
Copy link
Contributor Author

FauxFaux commented Jun 7, 2020

I think it's because we call dependencyTree 2,000 times still:

tsConfig: this.config.tsConfig,

..and each time it must compile the config. It is no longer compiling the config tens of times, but there's still 2,000 unnecessary compiles for us.

@pahen pahen merged commit fe3f468 into pahen:master Jun 8, 2020
@FauxFaux FauxFaux deleted the feat/tsconfig-perf branch June 8, 2020 07:21
@pahen
Copy link
Owner

pahen commented Jun 8, 2020

Thanks for your effort! Merged and published in Madge v3.9.1

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.

None yet

2 participants