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.

Sign up for GitHub

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

Jump to bottom

Pass absolute path to directoryExists #46086

Merged
merged 2 commits into from
Oct 21, 2021
Merged

Pass absolute path to directoryExists #46086

merged 2 commits into from
Oct 21, 2021

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Sep 27, 2021 •
edited
Loading

As pointed out by @gretzkiy, the directoryExists call added to
matchFiles in #44710 should have been passing the absolute path (since
the current directory might not match currentDirectory).

Fixes #45990

@amcasey
Pass absolute path to directoryExists
ba70c53 
As pointed out by @gretzkiy, the `directoryExists` call added to
`matchFiles` in microsoft#44710 should have been passing the absolute path (since
the current directory might not match `currentDirectory`).
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 27, 2021
@amcasey
Copy link
Member Author

amcasey commented Sep 27, 2021

Reported here.

@amcasey
Copy link
Member Author

amcasey commented Sep 27, 2021

@DanielRosenwasser @andrewbranch Do we want to include this in the next 4.4 patch?

@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 27, 2021
@andrewbranch
Copy link
Member

andrewbranch commented Sep 27, 2021 •
edited
Loading

The reason we thought there could be a mistake in getBasePaths/getFileMatcherPatterns is these lines:

// We also need to check the relative paths by converting them to absolute and normalizing
// in case they escape the base path (e.g "..\somedirectory")
const absolute: string = isRootedDiskPath(include) ? include : normalizePath(combinePaths(path, include));

which does not produce an absolute path if neither path nor include is absolute. What you’ve done is sufficient to fix the caller that we noticed is broken, and that may be enough—but the comments here do not give me confidence that this ever worked the way the author intended, and may be worth looking at more closely.

@DanielRosenwasser
Copy link
Member

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 27, 2021 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at ba70c53. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..46086

Metric main 46086 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 354,061k (± 0.02%) 354,059k (± 0.03%) -2k (- 0.00%) 353,821k 354,205k
Parse Time 1.94s (± 0.32%) 1.94s (± 0.57%) +0.00s (+ 0.05%) 1.92s 1.97s
Bind Time 0.84s (± 0.62%) 0.84s (± 0.87%) -0.00s (- 0.36%) 0.82s 0.85s
Check Time 5.48s (± 0.84%) 5.46s (± 0.52%) -0.02s (- 0.44%) 5.39s 5.53s
Emit Time 5.84s (± 1.26%) 5.84s (± 0.72%) +0.00s (+ 0.07%) 5.76s 5.96s
Total Time 14.10s (± 0.53%) 14.08s (± 0.38%) -0.02s (- 0.16%) 13.95s 14.18s
Compiler-Unions - node (v10.16.3, x64)
Memory used 203,850k (± 0.04%) 203,847k (± 0.02%) -4k (- 0.00%) 203,738k 203,977k
Parse Time 0.79s (± 0.85%) 0.79s (± 0.42%) +0.00s (+ 0.38%) 0.78s 0.80s
Bind Time 0.53s (± 1.13%) 0.53s (± 1.10%) +0.00s (+ 0.19%) 0.51s 0.54s
Check Time 7.87s (± 0.63%) 7.88s (± 0.56%) +0.01s (+ 0.14%) 7.79s 7.96s
Emit Time 2.45s (± 0.72%) 2.46s (± 0.79%) +0.00s (+ 0.16%) 2.43s 2.52s
Total Time 11.64s (± 0.42%) 11.66s (± 0.49%) +0.02s (+ 0.14%) 11.53s 11.75s
Monaco - node (v10.16.3, x64)
Memory used 342,015k (± 0.02%) 341,956k (± 0.02%) -59k (- 0.02%) 341,801k 342,097k
Parse Time 1.47s (± 0.35%) 1.48s (± 0.41%) +0.00s (+ 0.27%) 1.46s 1.49s
Bind Time 0.74s (± 0.67%) 0.75s (± 0.49%) +0.00s (+ 0.13%) 0.74s 0.75s
Check Time 5.44s (± 0.47%) 5.44s (± 0.84%) -0.00s (- 0.06%) 5.32s 5.56s
Emit Time 3.17s (± 0.83%) 3.18s (± 1.05%) +0.01s (+ 0.22%) 3.12s 3.27s
Total Time 10.83s (± 0.22%) 10.84s (± 0.48%) +0.01s (+ 0.08%) 10.71s 10.95s
TFS - node (v10.16.3, x64)
Memory used 304,620k (± 0.02%) 304,684k (± 0.01%) +64k (+ 0.02%) 304,627k 304,807k
Parse Time 1.20s (± 0.48%) 1.19s (± 0.57%) -0.00s (- 0.25%) 1.18s 1.21s
Bind Time 0.71s (± 0.82%) 0.71s (± 1.05%) +0.00s (+ 0.42%) 0.69s 0.72s
Check Time 4.96s (± 0.39%) 4.97s (± 0.66%) +0.01s (+ 0.22%) 4.91s 5.06s
Emit Time 3.29s (± 1.08%) 3.35s (± 1.00%) +0.05s (+ 1.64%) 3.25s 3.43s
Total Time 10.16s (± 0.49%) 10.22s (± 0.44%) +0.06s (+ 0.62%) 10.12s 10.33s
material-ui - node (v10.16.3, x64)
Memory used 472,183k (± 0.01%) 472,178k (± 0.01%) -4k (- 0.00%) 472,029k 472,301k
Parse Time 1.76s (± 0.46%) 1.76s (± 0.28%) +0.00s (+ 0.06%) 1.76s 1.78s
Bind Time 0.66s (± 0.61%) 0.66s (± 0.72%) -0.00s (- 0.30%) 0.65s 0.67s
Check Time 14.35s (± 0.44%) 14.41s (± 0.53%) +0.06s (+ 0.40%) 14.28s 14.64s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.78s (± 0.40%) 16.83s (± 0.46%) +0.05s (+ 0.32%) 16.70s 17.06s
Angular - node (v12.1.0, x64)
Memory used 331,946k (± 0.02%) 331,999k (± 0.03%) +54k (+ 0.02%) 331,852k 332,219k
Parse Time 1.94s (± 0.51%) 1.94s (± 0.73%) -0.00s (- 0.10%) 1.92s 1.98s
Bind Time 0.82s (± 0.71%) 0.82s (± 0.49%) +0.00s (+ 0.49%) 0.81s 0.83s
Check Time 5.29s (± 0.51%) 5.32s (± 0.51%) +0.03s (+ 0.62%) 5.24s 5.37s
Emit Time 6.13s (± 1.12%) 6.13s (± 0.66%) -0.00s (- 0.03%) 6.06s 6.22s
Total Time 14.18s (± 0.59%) 14.21s (± 0.34%) +0.03s (+ 0.23%) 14.06s 14.28s
Compiler-Unions - node (v12.1.0, x64)
Memory used 191,335k (± 0.09%) 191,345k (± 0.09%) +10k (+ 0.01%) 190,773k 191,563k
Parse Time 0.78s (± 0.93%) 0.78s (± 0.71%) -0.00s (- 0.51%) 0.77s 0.79s
Bind Time 0.53s (± 1.32%) 0.53s (± 1.40%) +0.00s (+ 0.19%) 0.52s 0.55s
Check Time 7.37s (± 0.45%) 7.38s (± 0.57%) +0.01s (+ 0.16%) 7.29s 7.49s
Emit Time 2.46s (± 0.52%) 2.45s (± 0.91%) -0.00s (- 0.20%) 2.41s 2.52s
Total Time 11.14s (± 0.26%) 11.14s (± 0.54%) +0.00s (+ 0.02%) 11.04s 11.28s
Monaco - node (v12.1.0, x64)
Memory used 325,117k (± 0.03%) 325,107k (± 0.02%) -9k (- 0.00%) 324,910k 325,266k
Parse Time 1.46s (± 0.47%) 1.46s (± 0.95%) +0.00s (+ 0.21%) 1.43s 1.48s
Bind Time 0.73s (± 0.84%) 0.73s (± 0.68%) -0.01s (- 0.68%) 0.72s 0.74s
Check Time 5.32s (± 0.39%) 5.35s (± 0.47%) +0.03s (+ 0.60%) 5.28s 5.40s
Emit Time 3.20s (± 0.70%) 3.20s (± 0.88%) +0.00s (+ 0.00%) 3.15s 3.25s
Total Time 10.71s (± 0.25%) 10.73s (± 0.39%) +0.03s (+ 0.23%) 10.63s 10.82s
TFS - node (v12.1.0, x64)
Memory used 289,361k (± 0.01%) 289,317k (± 0.02%) -45k (- 0.02%) 289,197k 289,527k
Parse Time 1.20s (± 0.88%) 1.21s (± 0.69%) +0.00s (+ 0.08%) 1.19s 1.22s
Bind Time 0.69s (± 0.69%) 0.69s (± 0.96%) +0.01s (+ 0.87%) 0.68s 0.71s
Check Time 4.91s (± 0.51%) 4.90s (± 0.46%) -0.01s (- 0.22%) 4.85s 4.96s
Emit Time 3.36s (± 0.90%) 3.34s (± 1.03%) -0.03s (- 0.77%) 3.28s 3.40s
Total Time 10.16s (± 0.38%) 10.13s (± 0.34%) -0.03s (- 0.31%) 10.07s 10.22s
material-ui - node (v12.1.0, x64)
Memory used 450,873k (± 0.02%) 450,827k (± 0.01%) -45k (- 0.01%) 450,713k 450,931k
Parse Time 1.77s (± 0.50%) 1.77s (± 0.53%) -0.01s (- 0.39%) 1.75s 1.79s
Bind Time 0.64s (± 1.04%) 0.64s (± 1.13%) -0.00s (- 0.16%) 0.62s 0.65s
Check Time 12.97s (± 0.55%) 13.09s (± 1.54%) +0.12s (+ 0.91%) 12.82s 13.83s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.38s (± 0.49%) 15.49s (± 1.28%) +0.11s (+ 0.72%) 15.24s 16.22s
Angular - node (v14.15.1, x64)
Memory used 330,419k (± 0.01%) 330,419k (± 0.01%) -0k (- 0.00%) 330,345k 330,454k
Parse Time 1.94s (± 0.52%) 1.96s (± 0.68%) +0.03s (+ 1.34%) 1.94s 1.99s
Bind Time 0.85s (± 0.43%) 0.86s (± 0.67%) +0.01s (+ 0.94%) 0.85s 0.88s
Check Time 5.34s (± 0.47%) 5.38s (± 0.55%) +0.04s (+ 0.65%) 5.32s 5.46s
Emit Time 6.20s (± 0.45%) 6.22s (± 0.74%) +0.02s (+ 0.31%) 6.13s 6.32s
Total Time 14.33s (± 0.32%) 14.42s (± 0.47%) +0.09s (+ 0.62%) 14.30s 14.63s
Compiler-Unions - node (v14.15.1, x64)
Memory used 191,283k (± 0.60%) 192,447k (± 0.49%) +1,165k (+ 0.61%) 189,969k 193,228k
Parse Time 0.81s (± 0.58%) 0.81s (± 0.84%) +0.00s (+ 0.49%) 0.80s 0.83s
Bind Time 0.56s (± 0.61%) 0.56s (± 0.67%) -0.00s (- 0.36%) 0.55s 0.56s
Check Time 7.51s (± 0.70%) 7.55s (± 0.83%) +0.04s (+ 0.59%) 7.41s 7.70s
Emit Time 2.43s (± 0.97%) 2.46s (± 0.91%) +0.03s (+ 1.11%) 2.41s 2.50s
Total Time 11.30s (± 0.61%) 11.38s (± 0.63%) +0.08s (+ 0.69%) 11.24s 11.53s
Monaco - node (v14.15.1, x64)
Memory used 323,971k (± 0.01%) 323,964k (± 0.01%) -8k (- 0.00%) 323,920k 323,997k
Parse Time 1.50s (± 0.54%) 1.51s (± 0.53%) +0.00s (+ 0.33%) 1.49s 1.52s
Bind Time 0.75s (± 0.63%) 0.75s (± 0.79%) +0.00s (+ 0.27%) 0.74s 0.77s
Check Time 5.32s (± 0.48%) 5.31s (± 0.54%) -0.01s (- 0.21%) 5.23s 5.38s
Emit Time 3.24s (± 0.86%) 3.21s (± 0.48%) -0.02s (- 0.68%) 3.17s 3.24s
Total Time 10.81s (± 0.42%) 10.78s (± 0.34%) -0.03s (- 0.28%) 10.68s 10.88s
TFS - node (v14.15.1, x64)
Memory used 288,306k (± 0.01%) 288,319k (± 0.01%) +13k (+ 0.00%) 288,261k 288,346k
Parse Time 1.22s (± 0.39%) 1.23s (± 0.61%) +0.01s (+ 0.82%) 1.22s 1.25s
Bind Time 0.73s (± 0.91%) 0.73s (± 0.76%) +0.00s (+ 0.27%) 0.72s 0.75s
Check Time 4.90s (± 0.38%) 4.90s (± 0.39%) -0.00s (- 0.06%) 4.85s 4.93s
Emit Time 3.48s (± 0.62%) 3.48s (± 0.68%) -0.00s (- 0.11%) 3.40s 3.51s
Total Time 10.33s (± 0.26%) 10.34s (± 0.33%) +0.01s (+ 0.07%) 10.25s 10.40s
material-ui - node (v14.15.1, x64)
Memory used 449,254k (± 0.01%) 449,250k (± 0.00%) -4k (- 0.00%) 449,224k 449,292k
Parse Time 1.83s (± 0.72%) 1.82s (± 0.46%) -0.01s (- 0.49%) 1.80s 1.83s
Bind Time 0.68s (± 0.77%) 0.68s (± 0.59%) 0.00s ( 0.00%) 0.67s 0.69s
Check Time 13.13s (± 0.39%) 13.16s (± 0.65%) +0.03s (+ 0.20%) 13.03s 13.36s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.64s (± 0.31%) 15.65s (± 0.58%) +0.02s (+ 0.10%) 15.52s 15.88s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory8 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
Benchmark Name Iterations
Current 46086 10
Baseline main 10

Developer Information:

Download Benchmark

@DanielRosenwasser
Copy link
Member

I think this needs a test, and I know you (@amcasey) are kind of swamped this week. We probably won't block the beta on this.

@andrewbranch andrewbranch assigned andrewbranch and unassigned amcasey Sep 30, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.5.1 milestone Sep 30, 2021
@andrewbranch
Copy link
Member

Added a test and moved the absolutification to where I think it should go. This allowed us to delete a parameter from the local visitDirectory function since patterns.basePaths is now guaranteed to be all-absolute.

@andrewbranch andrewbranch merged commit 55b4928 into microsoft:main Oct 21, 2021
@weswigham weswigham mentioned this pull request Nov 3, 2021
Merged
@amcasey amcasey deleted the DirExistsAbsolute branch November 3, 2021 20:57
andrewbranch added a commit to andrewbranch/TypeScript that referenced this pull request Nov 11, 2021
@andrewbranch
Revert "Pass absolute path to directoryExists (microsoft#46086)"
afef282 
This reverts commit 55b4928.
@andrewbranch andrewbranch mentioned this pull request Nov 11, 2021
Merged
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Nov 11, 2021
@andrewbranch @typescript-bot
Cherry-pick PR microsoft#46787 into release-4.5
8cc2c5f 
Component commits:
931b504 Revert "Fix RWC missing file detection (microsoft#46673)"
This reverts commit 4a065f5.

afef282 Revert "Pass absolute path to directoryExists (microsoft#46086)"
This reverts commit 55b4928.

f1a20b3 Revert "Reduce exceptions (microsoft#44710)"
This reverts commit c0d5c29.

56842cd Add back system watcher limit
andrewbranch added a commit that referenced this pull request Nov 11, 2021
@andrewbranch
Revert changes to matchFiles/readDirectory made since 4.3 (#46787)
1298f49 
* Revert "Fix RWC missing file detection (#46673)"

This reverts commit 4a065f5.

* Revert "Pass absolute path to directoryExists (#46086)"

This reverts commit 55b4928.

* Revert "Reduce exceptions (#44710)"

This reverts commit c0d5c29.

* Add back system watcher limit
andrewbranch added a commit that referenced this pull request Nov 11, 2021
@andrewbranch
Cherry-pick PR #46787 into release-4.5 (#46789)
460515f 
Component commits:
931b504 Revert "Fix RWC missing file detection (#46673)"
This reverts commit 4a065f5.

afef282 Revert "Pass absolute path to directoryExists (#46086)"
This reverts commit 55b4928.

f1a20b3 Revert "Reduce exceptions (#44710)"
This reverts commit c0d5c29.

56842cd Add back system watcher limit

Co-authored-by: Andrew Branch <[email protected]>
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
@amcasey @andrewbranch @mprobst
Pass absolute path to directoryExists (microsoft#46086)
0c0fb69 
* Pass absolute path to directoryExists

As pointed out by @gretzkiy, the `directoryExists` call added to
`matchFiles` in microsoft#44710 should have been passing the absolute path (since
the current directory might not match `currentDirectory`).

* Add test, simplify/clarify/fix matchFiles and friends

Co-authored-by: Andrew Branch <[email protected]>
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
@andrewbranch @mprobst
Revert changes to matchFiles/readDirectory made since 4.3 (microsoft#…
bdd324e 
…46787)

* Revert "Fix RWC missing file detection (microsoft#46673)"

This reverts commit 4a065f5.

* Revert "Pass absolute path to directoryExists (microsoft#46086)"

This reverts commit 55b4928.

* Revert "Reduce exceptions (microsoft#44710)"

This reverts commit c0d5c29.

* Add back system watcher limit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser approved these changes

@sandersn sandersn Awaiting requested review from sandersn

Assignees

@andrewbranch andrewbranch

@amcasey amcasey

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
PR Backlog
Archived in project
Milestone
TypeScript 4.5.1
Development

Successfully merging this pull request may close these issues.

Investigate undocumented readDirectory change
4 participants
@amcasey @andrewbranch @DanielRosenwasser @typescript-bot