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.

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

Core: Fix startup hang caused by watchStorySpecifiers #27016

Merged
merged 7 commits into from
Jun 13, 2024

Conversation

heyimalex
Copy link
Contributor

Closes #18881

What I did

Changed how watchpack gets initialized in watchStorySpecifiers. Previously we:

  • Passed in the directories from the story specifiers directly into watchpack as relative paths.

Now we:

  • Pass in the directories from the story specifiers and all nested files and directories.
  • Pass everything in as absolute paths instead of relative paths.

This fixes two issues:

  1. watchpack blocks for a long time closing FSWatchers on osx webpack/watchpack#222: Because we passed in only the top level directories, watchpack's recursive watching triggered a lot of churn once you cross the WATCHPACK_WATCHER_LIMIT threshold. The outcome was multi-second hangs on startup. See the repro in the linked issue, it's incredibly simple and reliably fails on OSX.
  2. Twice as many FSWatchers created when using storyStoreV7 #18881: Because we're now passing the same absolute paths to watchpack that webpack passes internally, it's able to internally re-use FSWatcher objects (given the same version of watchpack is being used under the hood).

Reproduce the original issue by checking out next and setting up a sandbox. cd into the sandbox.

Run yarn storybook and note the time to startup. On my machine, 334 ms for manager and 880 ms for preview.

Open node and run this to create a bunch of empty subdirectories within the sandbox.

const fs = require("fs");
const path = require("path");

const base = path.resolve("./src/extra");

// Generate a bunch of directories to simulate watchpack runs against.
function makeDirectoryTree(depth, dirsPerLevel) {
  fs.mkdirSync(base);
  let total = 0;
  let level = [base];
  for (let i = 0; i < depth; i++) {
    const next = [];
    for (const dir of level) {
      for (let j = 0; j < dirsPerLevel; j++) {
        const subdir = path.join(dir, `subdir-${i}-${j}`);
        fs.mkdirSync(subdir);
        total += 1;
        next.push(subdir);
      }
    }
    level = next;
  }
  console.log(`${total} directories created`);
}

makeDirectoryTree(5, 6);

Now run yarn storybook again. On my machine, 44 s for manager and 703 ms for preview.

Checkout this branch and run the test again. On my machine, 254 ms for manager and 669 ms for preview.

Checklist for Contributors

Testing

Updated some tests, didn't love how it went.

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

Did console.log testing / manual testing on the shape of everything in a sandbox.

Documentation

N/A

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

@chrisgarber
Copy link

Big savings 👍 🎉

@yannbf
Copy link
Member

yannbf commented May 6, 2024

Hey @heyimalex thanks a lot for your PR. So the storyStoreV7 flag doesn't exist anymore in Storybook 8. I'm assuming you are still facing the same issues in Storybook 8?

@tmeasday could you please take a look at this PR? Thanks!

@shilman
Copy link
Member

shilman commented May 6, 2024

@heyimalex thanks so much for putting this together. i just created a new PR that adds another Watchpack watcher. It's watching the config directory (e.g. .storybook/) and has slightly different logic than the current set of watchers. i don't know much about the performance implications of my work, but if you could possibly take a look and let me know if you have any concerns, i would really appreciate that!

@heyimalex heyimalex changed the title Prevent startup hang caused by storyStoreV7 Prevent startup hang caused by watchStorySpecifiers May 6, 2024
@heyimalex
Copy link
Contributor Author

@yannbf Sorry, I've updated the title. Yes, this is still an issue, it's been around for as long as watchStorySpecifiers has existed, but just now getting around to upstreaming it. We've had a similar patch in place at dayjob for almost two years now, back when storyStoreV7 was experimental 😅

@shilman I believe the same issue will exist, but it's IMO less likely to be problematic because .storybook doesn't tend to contain that many files. Default watcher limit on osx is ~2k, but that applies to all nested directories beneath the passed directory (if they don't match the ignore rules), so most folks will only see this issue when the size of their src / story containing dir becomes fairly large / deeply nested.

@tmeasday
Copy link
Member

tmeasday commented May 7, 2024

This seems great @heyimalex. Some questions:

Pass in the directories from the story specifiers and all nested files and directories.

I'm not really understanding why given we pass in the same set of directories it doesn't actually end up needing to recurse anyway, but it seems like you have a good handle on this :)

given the same version of watchpack is being used under the hood

I suppose there is no real way we can guarantee this, is there? But it is good that the perf boost is there in the case that it does happen.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM

Comment on lines +17 to +19
// Takes an array of absolute paths to directories and synchronously returns
// absolute paths to all existing files and directories nested within those
// directories (including the passed parent directories).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be sync? If we do it async might it give other parts of our boot up a chance to do work while waiting on FS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it must be sync, we'd just create the watcher asynchronously after directory walking completes. IMO performance implications here are not significant tho, even with 10k directories readdirSync calls seem to complete in negligible time.

@heyimalex
Copy link
Contributor Author

I'm not really understanding why given we pass in the same set of directories it doesn't actually end up needing to recurse anyway, but it seems like you have a good handle on this :)

Forgive me if this isn't perfectly accurate, haven't really worked on this in a couple of years.

So watchpack has direct watchers and recursive watchers. It has a function called reducePlan which takes a mapping of paths to watch, and intelligently collapses them down, creating recursive watchers to replace multiple direct watchers, so that the total number of watchers is below the limit.

When passing in all directories, this reduce plan runs once, and (given the repro conditions) watchpack will recognize that a single recursive watcher is the best move.

When passing in a single top level directory, essentially what happens is every time a readdir call completes, watchpack calls watch on each of the returned directories, and reducePlan runs again. You can see a "trace" of this behavior (created with hacky console log instrumenting) here. It ultimately gets to the same plan (see the final RecursiveWatcher.ctor: /Users/alex/projects/watchpack/fake-dir-base), but the process churns through the creation and close of a bunch of individual watchers, as reducePlan is working on an incomplete picture.

I suppose there is no real way we can guarantee this, is there? But it is good that the perf boost is there in the case that it does happen.

No, there isn't, this is more of a best-case-scenario optimization.

Also, just want to call out another possible solution: using a different library. The actual thing we're using watchpack for seems pretty simple on its face: we need to watch directories and trigger a callback when files that match some pattern nested within those directories are added, removed, or updated. Watchpack was probably seen as an easy enough way to get there, and something we already had a transitive dependency on, but given the level of footgunnery here maybe a different dep is appropriate.

@tmeasday
Copy link
Member

tmeasday commented May 7, 2024

Watchpack was probably seen as an easy enough way to get there, and something we already had a transitive dependency on, but given the level of footgunnery here maybe a different dep is appropriate.

Agree 💯 ! If you have another suggestion I'm all ears!

Copy link

nx-cloud bot commented May 7, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 80f8dc0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@heyimalex
Copy link
Contributor Author

After some light research, evaluating file watching libs is really hard hahaha. It seems like @parcel/watcher may be a good choice, as it's what's currently used by vscode. chokidar is widely deployed, but vscode moved away from it. Many other small libraries that seem to have spotty cross platform support or not be maintained.

@heyimalex
Copy link
Contributor Author

Alternative branch here, but this PR is definitely lower risk.

@tmeasday
Copy link
Member

tmeasday commented May 9, 2024

Thanks @heyimalex. We are in feature freeze right now but I'll aim to get this or the other one merged soon after 8.1 goes out.

@shilman shilman changed the title Prevent startup hang caused by watchStorySpecifiers Core: Fix startup hang caused by watchStorySpecifiers May 13, 2024
@shilman shilman self-assigned this May 13, 2024
@shilman
Copy link
Member

shilman commented May 13, 2024

@heyimalex thanks for your patience on this. we're releasing 8.1 in a day or two and i'm planning to merge this as soon as we get that out so it has some time to bake for 8.2. excellent work!!! 🙌

@tmeasday
Copy link
Member

@shilman we should consider the other PR that migrates us to Parcel's file watcher.

@shilman
Copy link
Member

shilman commented May 14, 2024

Greate call @tmeasday -- I totally missed that. Let's discuss at the next available opportunity!

@shilman shilman merged commit dd69fd3 into storybookjs:next Jun 13, 2024
50 of 51 checks passed
@github-actions github-actions bot mentioned this pull request Jun 13, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Twice as many FSWatchers created when using storyStoreV7
5 participants