Conversation
OverviewAnalysis of 39,976 functions in Nushell (3,004 modified, 5,224 new, 5,218 removed, 26,530 unchanged) reveals mixed performance impact from environment variable refactoring across three commits implementing case-insensitive handling on Windows. Binary target.aarch64-unknown-linux-gnu.release.nu shows +0.248% power consumption (+2,294 nJ). Changes introduce EnvName wrapper type replacing O(n) linear scans with O(1) HashMap lookups, achieving correctness improvements with acceptable performance trade-offs. Function AnalysisMajor Regressions (Job Control System):
Major Improvements (Command System):
Hot-Path Optimization:
Direct Optimization:
Other analyzed functions in error handling, type comparison, and compiler-generated code showed minor changes with limited practical impact. Additional FindingsJob control regressions (373-545%) occur in non-critical paths outside primary hot paths (IR execution, pipeline flow, parser, REPL loop). The 42-43µs absolute increases are acceptable for cleanup operations. Error handling shows net positive impact: guard functions improve 90-98% (executing frequently) while diagnostic functions regress 128-176% (executing rarely). The architectural refactoring successfully optimizes primary use cases while accepting minor secondary system regressions, justified by cross-platform correctness requirements. 🔎 Full breakdown: Loci Inspector. |
1c6453c to
9e52565
Compare
410f1b0 to
1e109b2
Compare
6e93079 to
68fe8e6
Compare
5184682 to
8d99fe1
Compare
d6c3b3e to
aa2e4e6
Compare
Note
Source pull request: nushell/nushell#17558
Apparently, there are still edge cases for case-insensitive env vars in Windows, so someone pointed me a different way to try and do it. This PR represents that way. All env vars (not just path) will be treated case-insensitive on Windows and case-sensitive on all other operating systems. Since this does the sensitivity checking for us, I removed (commented out for now) the get_env_var_insensitive function. I realize that windows is case-preserving and not really case-insensitive but this seemed like the best option for now.
Let me know if this is a bad idea.
Release notes summary - What our users need to know
Tasks after submitting