- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.6k
 
Optimize recursive ls #2083
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
          
     Merged
      
      
    
      
        
          +179
        
        
          −71
        
        
          
        
      
    
  
  
     Merged
                    Optimize recursive ls #2083
Changes from 6 commits
      Commits
    
    
            Show all changes
          
          
            11 commits
          
        
        Select commit
          Hold shift + click to select a range
      
      5850f78
              
                ls: Remove allocations by eliminating collect/clones
              
              
                siebenHeaven 76cc7fb
              
                ls: Introduce PathData structure
              
              
                siebenHeaven af68ec8
              
                ls: Cache more data related to paths
              
              
                siebenHeaven a7bbef9
              
                ls: Add BENCHMARKING.md
              
              
                siebenHeaven f89cb6d
              
                ls: Document PathData structure
              
              
                siebenHeaven b4af0d0
              
                tests/ls: Add testcase for error paths with width option
              
              
                siebenHeaven 5731dc4
              
                ls: Fix unused import warning
              
              
                siebenHeaven fc66b32
              
                ls: Suggest checking syscall count in BENCHMARKING.md
              
              
                siebenHeaven 34b4ae0
              
                ls: Remove mentions of sort in BENCHMARKING.md
              
              
                siebenHeaven 9415f6c
              
                ls: Remove dependency on cached
              
              
                siebenHeaven 42ecac3
              
                ls: Fix MSRV error related to map_or
              
              
                siebenHeaven File filter
Filter by extension
Conversations
          Failed to load comments.   
        
        
          
      Loading
        
  Jump to
        
          Jump to file
        
      
      
          Failed to load files.   
        
        
          
      Loading
        
  Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
      
      Oops, something went wrong.
      
    
  
  
    
      This file contains hidden or 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
    
  
  
    
              | Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # Benchmarking ls | ||
| 
     | 
||
| ls majorly involves fetching a lot of details (depending upon what details are requested, eg. time/date, inode details, etc) for each path using system calls. Ideally, any system call should be done only once for each of the paths - not adhering to this principle leads to a lot of system call overhead multiplying and bubbling up, especially for recursive ls, therefore it is important to always benchmark multiple scenarios. | ||
| This is an overwiew over what was benchmarked, and if you make changes to `ls`, you are encouraged to check | ||
| how performance was affected for the workloads listed below. Feel free to add other workloads to the | ||
| list that we should improve / make sure not to regress. | ||
| 
     | 
||
| Run `cargo build --release` before benchmarking after you make a change! | ||
| 
     | 
||
| ## Simple recursive ls | ||
| 
     | 
||
| - Get a large tree, for example linux kernel source tree. | ||
| - Benchmark simple recursive ls with hyperfine: `hyperfine --warmup 2 "target/release/coreutils ls -R tree > /dev/null"`. | ||
| 
     | 
||
| ## Recursive ls with all and long options | ||
| 
     | 
||
| - Same tree as above | ||
| - Benchmark recursive ls with -al -R options with hyperfine: `hyperfine --warmup 2 "target/release/coreutils ls -al -R tree > /dev/null"`. | ||
| 
     | 
||
| ## Comparing with GNU sort | ||
| 
     | 
||
| Hyperfine accepts multiple commands to run and will compare them. To compare performance with GNU sort | ||
| duplicate the string you passed to hyperfine but remove the `target/release/coreutils` bit from it. | ||
| 
     | 
||
| Example: `hyperfine --warmup 2 "target/release/coreutils ls -al -R tree > /dev/null"` becomes | ||
| `hyperfine --warmup 2 "target/release/coreutils ls -al -R tree > /dev/null" "ls -al -R tree > /dev/null"` | ||
| (This assumes GNU sort is installed as `sort`) | ||
| 
     | 
||
| This can also be used to compare with version of ls built before your changes to ensure your change does not regress this | ||
  
    
      This file contains hidden or 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
    
  
  
    
              
      
      Oops, something went wrong.
        
    
  
  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.
  
    
  
    
Uh oh!
There was an error while loading. Please reload this page.