feat/issue-306-pr #965

zamu-flowerpot · 2022-11-09T22:41:26Z

This would resolve issue #306 with entry counts for directories.
This also resolves issue #964 as I had to define how to compare EntrySize::EntryCount and EntrySize::Bytes.

This adds a single dependency walkdir to handle counting the number of items in a directory cross-platform. Also, by default it doesn't follow symbolic links which seems fine.

`src/listing.rs` === * Add `EntrySize` struct * Impl Display for EntrySize * Change `Option<bytesize::ByteSize>` to `EntrySize` in `Entry` * Add Entry::Directory size calculation utilizing WalkDir * Sort directory listings by always putting directories at the top and sorting files and directories based on their own semantics (ie. entry counts for directories and bytes for files) `src/renderer.rs` === * Change `entry_raw` from using `if-let` to just operating on `entry.size`

zamu-flowerpot · 2022-11-09T22:53:58Z

One thing I didn't look into was setting these behind CLI flags and this probably needs some memoization as it can be just slightly noticeable when looking at parents with a lot of children (or maybe just limiting it down to direct children).

svenstaro · 2022-11-10T12:12:29Z

I think this should probably be behind CLI flags as the performance hit can be severe and we don't want that by default. I will do a proper review a little later.

svenstaro · 2022-11-10T12:16:15Z

Cool stuff! As you already said, a memoization technique would be neat. I think a HashMap would be good enough for us. What do you also think about timing out on directories that just take too long? I don't miniserve to ever not feel snappy when generating pages.

svenstaro · 2022-11-10T12:13:05Z

src/renderer.rs

+                            // @if let size = entry.size {
+                            //     span.mobile-info.size {
+                            //         (maud::display(size))
+                            //     }
+                            // }
                        }
                    }
                }
            }
            td.size-cell {
-                @if let Some(size) = entry.size {
-                    (maud::display(size))
-                }
+                (maud::display(&entry.size))
+                // @if let size = entry.size {
+                //     (maud::display(size))
+                // }


What's with the commented code here?

Ah, I didn't remove that since it was the code that would need to be switched on if we were going to throw everything behind a feature flag.

svenstaro · 2022-11-10T12:13:18Z

src/listing.rs

+            // e2.size
+            //     .unwrap_or_else(|| ByteSize::b(0))
+            //     .cmp(&e1.size.unwrap_or_else(|| ByteSize::b(0)))


svenstaro · 2022-11-10T12:14:25Z

src/listing.rs

+#[derive(Clone, Copy, PartialEq, Eq)]
+/// Possible entry size types
+pub enum EntrySize {
+    /// EntryCount is number of entries in a directory
+    EntryCount(usize),
+    /// Bytes is number of bytes in a file
+    Bytes(bytesize::ByteSize),
+}
+
+impl Display for EntrySize {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            EntrySize::EntryCount(count) => write!(f, "{}", count),
+            EntrySize::Bytes(bytes) => write!(f, "{}",  bytes),
+        }
+    }
+}
+


Wouldn't it be cool to also show the combined directory file size?

Not sure what you are referring to by "combined directory file size". Are you referring to the total size in bytes of a directory? That might be a bit expensive and probably would require a persistent cache to be anywhere near fast.

Yes, the byte sizes. I recognize it might be too slow. Hm. If you like to, you could try to experiment with this.

zamu-flowerpot · 2022-11-10T13:39:10Z

Cool stuff! As you already said, a memoization technique would be neat. I think a HashMap would be good enough for us. [...]

A HashMap would be fine. I tried my hand at using a once_cell::OnceCell<HashMap>, but couldn't figure out how to make a mutable global static. Maybe it'd just be as simple as just constructing it and passing it in as part of the function call to directory_listing?

[...] What do you also think about timing out on directories that just take too long? I don't miniserve to ever not feel snappy when generating pages.

Definitely. We might be able to shortcut a lot of the issues by just counting direct children by changing the size calculation to call .max_depth(1). It still might be slow, but it's on the user if they have a directory with a million files right?

zamu-flowerpot · 2022-11-10T13:42:04Z

src/listing.rs

@@ -256,12 +276,14 @@ pub fn directory_listing(
                    Err(_) => None,
                };

+                let size = WalkDir::new(entry.path()).into_iter().count();


Thinking about it, we might be able to just do .take(UPPER_LIMIT).count() instead of .count() to shortcut execution time. So, we might have UPPER_LIMIT be something like 10001 and if it's more than 10000 we have the Impl Display for EntrySize print >10000 or something along those lines.

The other option would have been to add an actual timeout, but I'm not really sure how to best do that in a non-async context.

svenstaro · 2022-11-11T03:14:00Z

Cool stuff! As you already said, a memoization technique would be neat. I think a HashMap would be good enough for us. [...]

A HashMap would be fine. I tried my hand at using a once_cell::OnceCell<HashMap>, but couldn't figure out how to make a mutable global static. Maybe it'd just be as simple as just constructing it and passing it in as part of the function call to directory_listing?

You're going to need a OnceCell<Arc<Mutex<HashMap>>>.

[...] What do you also think about timing out on directories that just take too long? I don't miniserve to ever not feel snappy when generating pages.

Definitely. We might be able to shortcut a lot of the issues by just counting direct children by changing the size calculation to call .max_depth(1). It still might be slow, but it's on the user if they have a directory with a million files right?

How about walking through the tree in threads using rayon? Should be fairly easy and it might also allow you to cancel/timeout threads.

`Cargo.lock` & `Cargo.toml` === * Remove `WalkDir` * Add `fs_extra` * Add `OnceCell` `src/listing.rs` === * remove `use walkdir::WalkDir` * add HashMap, Arc, Mutex, OnceCell, log::warn, and `FILE_SIZE_CACHE` * Cargo fmt

`src/listing.rs` === * Cargo fmt * Replace `WalkDir::new(entry.path()).into_iter().count()` with calculating directory size using `fs_extra::dir::get_size` * If `get_size` fails, fall back to `std::fs::read_dir` counting

zamu-flowerpot · 2022-11-21T01:51:31Z

You're going to need a OnceCell<Arc<Mutex<HashMap>>>.

Yep, worked like a charm. Thanks for the hint.

How about walking through the tree in threads using rayon? Should be fairly easy and it might also allow you to cancel/timeout threads.

Tried rayon and then encountered BurntSushi/walkdir#21. This lead me to look for something other than WalkDir and found fs_extra.

I just wanted to get this to a MVP and figure out what flags we want to incorporate (and how to wire them from the config/cli to the call site). So for now I left off the parallel scanning.

Looking at fs_extras code, it might be doable to parallelize the size scanning using Rayon. If we get all the decedent DirEntrys and push them onto a stack, for_each over the stack and cache their size, and then calculate the directory sizes from the cache on request, we should be pretty fast. The biggest issue still comes down to cache invalidation though it wouldn't be too hard to change the cache map to include modified time.

That said another option is to spawn a thread that listens for file system notifications and updates the cache on writes, but I'm only aware of Linux's inotify so I'm not sure if it's really cross-platform.

Let me know if you have any questions or concerns.

zamu-flowerpot added 2 commits November 9, 2022 17:29

Add 'WalkDir' dependency

c1cc958

svenstaro reviewed Nov 10, 2022

View reviewed changes

zamu-flowerpot commented Nov 10, 2022

View reviewed changes

zamu-flowerpot added 3 commits November 20, 2022 20:17

feat/issue-306-pr:

1c71e93

`Cargo.lock` & `Cargo.toml` === * Remove `WalkDir` * Add `fs_extra` * Add `OnceCell` `src/listing.rs` === * remove `use walkdir::WalkDir` * add HashMap, Arc, Mutex, OnceCell, log::warn, and `FILE_SIZE_CACHE` * Cargo fmt

feat/issue-306-pr:

dc734c4

`src/listing.rs` === * Cargo fmt * Replace `WalkDir::new(entry.path()).into_iter().count()` with calculating directory size using `fs_extra::dir::get_size` * If `get_size` fails, fall back to `std::fs::read_dir` counting

Merge branch 'master' into feat/issue-306-pr

70f8b3e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/issue-306-pr #965

feat/issue-306-pr #965

zamu-flowerpot commented Nov 9, 2022

zamu-flowerpot commented Nov 9, 2022 •

edited

Loading

svenstaro commented Nov 10, 2022

svenstaro commented Nov 10, 2022

svenstaro Nov 10, 2022

zamu-flowerpot Nov 10, 2022

svenstaro Nov 10, 2022

svenstaro Nov 10, 2022

zamu-flowerpot Nov 10, 2022

svenstaro Nov 11, 2022

zamu-flowerpot commented Nov 10, 2022

zamu-flowerpot Nov 10, 2022

zamu-flowerpot Nov 10, 2022

svenstaro commented Nov 11, 2022

zamu-flowerpot commented Nov 21, 2022 •

edited

Loading

feat/issue-306-pr #965

Are you sure you want to change the base?

feat/issue-306-pr #965

Conversation

zamu-flowerpot commented Nov 9, 2022

zamu-flowerpot commented Nov 9, 2022 • edited Loading

svenstaro commented Nov 10, 2022

svenstaro commented Nov 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zamu-flowerpot commented Nov 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenstaro commented Nov 11, 2022

zamu-flowerpot commented Nov 21, 2022 • edited Loading

zamu-flowerpot commented Nov 9, 2022 •

edited

Loading

zamu-flowerpot commented Nov 21, 2022 •

edited

Loading