feat: move from notify v4 to notify-debouncer-full v0.3 #2503

iamorphen · 2024-05-20T01:21:52Z

Introduction

Closes #2077 , unblocks #2498 .

This updates our notify dependency from v4 to notify-debouncer-full v0.3. The newer notify libraries behave significantly differently from the version zola currently uses, so we need to introduce more logic to generally preserve the current end-user experience behind the scenes. The most significant difference is that serve now needs to react to a Vec<DebouncedEvent> instead of a single DebouncedEvent.

Code Changes

Use notify-debouncer-full, which adds debouncing functionality peripheral to the newer notify v6. This functionality was built directly into v4.
Map fine-grained notify file system event kinds into our own local abstraction for ease of use.
Introduce get_relevant_event_kind to map the vector of notify file system events to our abstraction and filter out events we aren't interested in. This moves previously inlined filtering logic into its own function. The new function lives in a new library, components/fs_event_utils.
Sort the incoming events by timestamp, descending. Add the events into a hash map where the key is the event path. This way we keep only the latest event for each unique path. This helps in cases where editors like Vim raise multiple events (which don't seem to be debounced) on a single path.
Munge the collection in (4) into a new map that bins changes by change type.
Iterate over the change map from (5) to process the group of events. This means we can make multiple changes to the site in rapid succession if the user changes a lot of files within the debounce period.
reload_sass and reload_templates now pass an absolute path to the sass and templates directories, respectively, to rebuild_done_handling to participate in livereload. To support this, the Site struct now stores the sass and template directories as fields.

Testing

Unit Tests

This MR introduces a new unit test to test the new internal support function listed above. Run with cargo test --package fs_event_utils.

Integration Testing

I created the following script to rapidly modify test_site.

Expand for script.

# Config
echo " " >> test_site/config.toml
echo " " >> test_site/config.toml

# Content
echo " " >> test_site/content/root-page-1.md
echo " " >> test_site/content/posts/fixed-slug.md

# Templates
echo " " >> test_site/templates/index.html
echo " " >> test_site/templates/page.html

# Static files
echo " " >> test_site/static/site.css
echo " " >> test_site/static/scripts/hello.js

# SASS
echo " " >> test_site/sass/blog.scss
echo " " >> test_site/sass/scss.scss

# Themes
echo " " >> test_site/themes/sample/theme.toml
echo " " >> test_site/themes/sample/sass/sample.scss

Here is the result of running the test on zola built on next.

Expand for results on next.

Change detected @ 2024-05-19 20:15:53
-> Config changed. The browser needs to be refreshed to make the changes visible.
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 35 pages (1 orphan) and 12 sections
Done in 87ms.

Change detected @ 2024-05-19 20:15:54
-> Content changed /Users/orphen/code/zola/test_site/content/root-page-1.md
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 35 pages (1 orphan) and 12 sections
Done in 46ms.

Change detected @ 2024-05-19 20:15:54
-> Content changed /Users/orphen/code/zola/test_site/content/posts/fixed-slug.md
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 35 pages (1 orphan) and 12 sections
Done in 45ms.

Change detected @ 2024-05-19 20:15:54
-> Template changed /Users/orphen/code/zola/test_site/templates/index.html
Reloading only template
Done in 20ms.

Change detected @ 2024-05-19 20:15:54
-> Template changed /Users/orphen/code/zola/test_site/templates/page.html
Reloading only template
Done in 20ms.

Change detected @ 2024-05-19 20:15:54
-> Static file changed /Users/orphen/code/zola/test_site/static/site.css
Done in 0ms.

Change detected @ 2024-05-19 20:15:54
-> Static file changed /Users/orphen/code/zola/test_site/static/scripts/hello.js
Done in 0ms.

Change detected @ 2024-05-19 20:15:54
-> Sass file changed /Users/orphen/code/zola/test_site/sass/blog.scss
Done in 0ms.

Change detected @ 2024-05-19 20:15:54
-> Sass file changed /Users/orphen/code/zola/test_site/sass/scss.scss
Done in 0ms.

Change detected @ 2024-05-19 20:15:54
-> Themes changed.
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 35 pages (1 orphan) and 12 sections
Done in 44ms.

Change detected @ 2024-05-19 20:15:54
-> Themes changed.
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 35 pages (1 orphan) and 12 sections
Done in 45ms.

The config changes are rolled into a single update, and the content, template, static, sass, and theme changes are all processed individually (2 changes each).

Cumulative time to update is ~310 ms.

Here is the result of running the test on this feature branch.

Expand for test result on this branch.

Change detected @ 2024-05-20 09:10:48
-> Template file(s) changed /Users/orphen/code/zola/test_site/templates/index.html, /Users/orphen/code/zola/test_site/templates/page.html
Reloading only template
Done in 49ms.

Change detected @ 2024-05-20 09:10:48
-> Static file changed /Users/orphen/code/zola/test_site/static/scripts/hello.js
-> Static file changed /Users/orphen/code/zola/test_site/static/site.css
Done in 0ms.

Change detected @ 2024-05-20 09:10:48
-> Themes changed.
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 35 pages (1 orphan) and 12 sections
Done in 54ms.

Change detected @ 2024-05-20 09:10:48
-> Sass file(s) changed /Users/orphen/code/zola/test_site/sass/blog.scss, /Users/orphen/code/zola/test_site/sass/scss.scss
Done in 1ms.

Change detected @ 2024-05-20 09:10:48
-> Config changed. The browser needs to be refreshed to make the changes visible.
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 35 pages (1 orphan) and 12 sections
Done in 43ms.

Change detected @ 2024-05-20 09:10:48
-> Content changed /Users/orphen/code/zola/test_site/content/posts/fixed-slug.md
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 35 pages (1 orphan) and 12 sections
-> Content changed /Users/orphen/code/zola/test_site/content/root-page-1.md
Checking all internal links with anchors.
> Successfully checked 0 internal link(s) with anchors.
-> Creating 35 pages (1 orphan) and 12 sections
Done in 87ms.

(The order of events is different, subject mostly to OS event scheduling and whatever the backing notify library is at the mercy of.) Template changes are batched into a single update, sass changes are batched into a single update, themes changes are batched into a single update, the config changes are still rolled into 1, and the static files and content are processed individually (looped over).

Cumulative time to update is ~235 ms. This single test is by no means a reliable profiling, but the results are not surprising, since we perform fewer operations overall relative to current behavior on next.

Manual Tests

I modified my own personal site using zola built from this branch; in particular, I edited files with Vim. This is what revealed the need for debounce_events.

Alternatives

It is possible we perform multiple site rebuilds in one debounce period, for example by modifying the config and modifying the theme. I investigated adding an optimization that would skip all remaining event processing in a debounce period if some change requires a site rebuild. While doable, this handling greatly complicates the logic in my opinion.

First, we can no longer simply match on ChangeKind because we would need to prioritize handling of particular events. This is mostly done by a branch-order-matters if that first checks for config changes, then template changes, then etc. In the case that we don't do a site rebuild in a debounce period, we have to make sure we correctly apply all required updates to account for each change.

If we do a site rebuild, we also need a clean way to end event processing early. We can do this with labeled match cases and break to those labels, but we'd also need to account for printing the time taken to apply site changes. Time-tracking and breaking at various points during the event handling makes understanding control flow more difficult.

To keep the user-facing experience similar with what's on master, we would also need to let the end-user know which files we observed changing, and the reporting of such becomes non-trivial (ex: if config changed, print usual config-changed message and then list all other files; but if templates changed, print usual templates-changed message and then list all other files; but if you don't rebuild the site and only do partial updates , then...).

I don't know whether end users are often applying many site changes in a single debounce window. I personally do not. I don't think pursuing this optimization is worth it from an ROI-to-end-user and maintainability perspective.

iamorphen · 2024-05-20T01:44:20Z

src/cmd/serve.rs

        console::info(&msg);
        rebuild_done_handling(
            &broadcaster,
            compile_sass(&site.base_path, &site.output_path),
-            &partial_path.to_string_lossy(),
+            &site.sass_path.to_string_lossy(),


@Keats , a question on this change and a similar one in reload_templates. Previously we passed partial paths here, but now I'm passing an absolute path to the root of the sass directory. I couldn't find (or resolve, for some reason) protocol docs for livereload v7. Is this change okay? Functionally it seems to work.

I have no idea. If it works it's probably fine, i'll try it locally.

Keats · 2024-05-20T20:14:27Z

src/cmd/serve.rs

+                    assert!(
+                        event.event.paths.len() == 1,
+                        "Didn't expect notify event with multiple paths."
+                    );


Either get_relevant_event_kind should make sure of that or we should return an error rather than a panic

Fixed. It now prints an error and skips that event.

Keats · 2024-05-20T20:27:19Z

src/cmd/serve.rs

+
+                // Map of change_kind -> (partial_path, full_path, event_kind).
+                let changes: HashMap<ChangeKind, Vec<(PathBuf, &PathBuf, &SimpleFSEventKind)>> =
+                    map_events_to_change_map(&filtered_events, &root_dir, &config_path);


Can we extract the whole logic of finding and sorting/filtering events to a separate function so it's easy to test and doesn't take space in the actual loop?

Thanks for your time. Before I start, to confirm, are you asking about moving only the new code in this merge request or also existing code like is_ignored_file, is_temp_file, etc. participating in the filtering?

I think it might make sense to move everything to a new file? But no strong feeling about it

Thanks. In that case, I will focus on just my changes for now.

Keats · 2024-05-20T20:34:34Z

src/cmd/serve.rs

+                        ChangeKind::Templates => {
+                            let partial_paths: Vec<&PathBuf> =
+                                change_group.iter().map(|(p, _, _)| p).collect();
+                            let full_paths: Vec<&PathBuf> =


when we do get partial and full paths?

If I understand your question correctly, we get full paths from the incoming events from notify, and we get partial paths from detect_change_kind. In the implementation on next, check the partial paths to determine whether shortcodes were modified. We pass the full paths to reload_templates. I'm not sure if the latter is necessary, so just following what I see on next.

Keats · 2024-05-20T20:37:09Z

src/cmd/serve.rs

+
+/// Return a map storing all events of a like `ChangeKind` behind one key.
+/// Each element maps `change_kind -> (partial_path, full_path, event_kind)`.
+fn map_events_to_change_map<'a>(


let's move all that code to another file

I've introduced components/fs_event_utils. I took a guess at where to put code. If there's a better place, please advise.

Let's put it in the root component, next to cli.rs

Keats · 2024-05-20T20:40:44Z

src/cmd/serve.rs

+/// with the latest `Instant` value.
+fn debounce_events(
+    events: &Vec<(PathBuf, SimpleFSEventKind, Instant)>,
+) -> Vec<(PathBuf, SimpleFSEventKind)> {


Assuming the events are ordered (or that can will order them), we could just use a hashmap with the path as the key and insert them in order

Okay, thanks. I took the liberty to use a hash map earlier on, and that allowed me to remove two of the functions I introduced. Upside is that the diff is smaller, downside is that we lost some test guarantees because some logic is inlined into serve.

iamorphen

I have updated the description to match the implementation of the latest update. I re-ran my manual test and confirmed the same behavior.

iamorphen · 2024-05-21T12:05:21Z

src/cmd/serve.rs

+                        ChangeKind::Templates => {
+                            let partial_paths: Vec<&PathBuf> =
+                                change_group.iter().map(|(p, _, _)| p).collect();
+                            let full_paths: Vec<&PathBuf> =


If I understand your question correctly, we get full paths from the incoming events from notify, and we get partial paths from detect_change_kind. In the implementation on next, check the partial paths to determine whether shortcodes were modified. We pass the full paths to reload_templates. I'm not sure if the latter is necessary, so just following what I see on next.

iamorphen · 2024-05-21T12:06:09Z

src/cmd/serve.rs

+                    assert!(
+                        event.event.paths.len() == 1,
+                        "Didn't expect notify event with multiple paths."
+                    );


Fixed. It now prints an error and skips that event.

iamorphen · 2024-05-21T12:06:28Z

src/cmd/serve.rs

+
+                // Map of change_kind -> (partial_path, full_path, event_kind).
+                let changes: HashMap<ChangeKind, Vec<(PathBuf, &PathBuf, &SimpleFSEventKind)>> =
+                    map_events_to_change_map(&filtered_events, &root_dir, &config_path);


iamorphen · 2024-05-21T12:08:22Z

src/cmd/serve.rs

+/// with the latest `Instant` value.
+fn debounce_events(
+    events: &Vec<(PathBuf, SimpleFSEventKind, Instant)>,
+) -> Vec<(PathBuf, SimpleFSEventKind)> {


Okay, thanks. I took the liberty to use a hash map earlier on, and that allowed me to remove two of the functions I introduced. Upside is that the diff is smaller, downside is that we lost some test guarantees because some logic is inlined into serve.

iamorphen · 2024-05-21T12:09:02Z

src/cmd/serve.rs

+
+/// Return a map storing all events of a like `ChangeKind` behind one key.
+/// Each element maps `change_kind -> (partial_path, full_path, event_kind)`.
+fn map_events_to_change_map<'a>(


I've introduced components/fs_event_utils. I took a guess at where to put code. If there's a better place, please advise.

Keats · 2024-05-22T18:33:01Z

LGTM. I'll make some changes to the code org later but it works fine locally. Thanks a lot!

iamorphen · 2024-05-22T22:00:21Z

Thanks Keats!

Keats · 2024-05-23T19:49:02Z

Here's the commit i've added: b693e62
Just moving more things to that new file to try to keep the serve.rs reasonable

iamorphen · 2024-05-24T01:02:25Z

Nice, thank you for the context and refactor.

iamorphen marked this pull request as ready for review May 20, 2024 01:38

iamorphen commented May 20, 2024

View reviewed changes

Keats requested changes May 20, 2024

View reviewed changes

iamorphen force-pushed the feat/orphen/integrate-debouncer-full branch from 4e7a725 to f26fd10 Compare May 21, 2024 12:00

iamorphen commented May 21, 2024

View reviewed changes

feat: move from notify v4 to notify-debouncer-full

cf386fe

iamorphen force-pushed the feat/orphen/integrate-debouncer-full branch from f26fd10 to cf386fe Compare May 22, 2024 06:52

Keats merged commit e7bbd3f into getzola:next May 22, 2024
5 checks passed

iamorphen deleted the feat/orphen/integrate-debouncer-full branch May 22, 2024 22:00

Keats pushed a commit that referenced this pull request Jun 20, 2024

feat: move from notify v4 to notify-debouncer-full (#2503)

5b3a57b

berdandy pushed a commit to berdandy/azola that referenced this pull request Sep 17, 2024

feat: move from notify v4 to notify-debouncer-full (getzola#2503)

b85ac2e

jamiedumont mentioned this pull request Oct 5, 2024

Bug: File watching during zola serve failing with Zola 19.2 on macOS 14.6.1 #2663

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move from notify v4 to notify-debouncer-full v0.3 #2503

feat: move from notify v4 to notify-debouncer-full v0.3 #2503

iamorphen commented May 20, 2024 •

edited

Loading

iamorphen May 20, 2024

Keats May 20, 2024

Keats May 20, 2024

iamorphen May 21, 2024

Keats May 20, 2024

iamorphen May 20, 2024

Keats May 21, 2024

iamorphen May 21, 2024

iamorphen May 21, 2024

Keats May 20, 2024

iamorphen May 21, 2024

Keats May 20, 2024

iamorphen May 21, 2024

Keats May 21, 2024

iamorphen May 22, 2024

Keats May 20, 2024

iamorphen May 21, 2024

iamorphen left a comment

iamorphen May 21, 2024

iamorphen May 21, 2024

iamorphen May 21, 2024

iamorphen May 21, 2024

iamorphen May 21, 2024

Keats commented May 22, 2024

iamorphen commented May 22, 2024

Keats commented May 23, 2024

iamorphen commented May 24, 2024

feat: move from notify v4 to notify-debouncer-full v0.3 #2503

feat: move from notify v4 to notify-debouncer-full v0.3 #2503

Conversation

iamorphen commented May 20, 2024 • edited Loading

Introduction

Code Changes

Testing

Unit Tests

Integration Testing

Manual Tests

Alternatives

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

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

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

iamorphen left a 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

Choose a reason for hiding this comment

Keats commented May 22, 2024

iamorphen commented May 22, 2024

Keats commented May 23, 2024

iamorphen commented May 24, 2024

iamorphen commented May 20, 2024 •

edited

Loading