benatshippabo
commented
1 year ago So weird that we exceed 16777216 entries in the set, maybe there is a bug in invalidating set entries 🤔
Closed fc closed 1 year ago
benatshippabo
commented
1 year ago So weird that we exceed 16777216 entries in the set, maybe there is a bug in invalidating set entries 🤔
benatshippabo
commented
1 year ago actually yeah, @sapphi-red isn't this a bug?
Set are not smart enough to deduplicate objects based on their property, for this to work as intended, the entries we want deduplicated would have to reference the same variable
sapphi-red
commented
1 year ago By digging down the history, it seems this Set is intended to be used to dedupe the boundaries variable.
https://github.com/vitejs/vite/commit/0974dbeb7f4de03ff7c6091ca1a507470a0f6977#diff-54add59d921f03d4386f04ed8726faa5634abb4898f3e5a14f2c641431475781
I think it's a bug. It needs to be something like:
const boundaries = new Map<ModuleNode, Set<ModuleNode>>()
const s = boundaries.get(boundary) ?? new Set()
s.add(acceptedVia)
boundaries.set(boundary, s)
github-actions[bot]
commented
1 year ago Hello @fc. We like your proposal/feedback and would appreciate a contribution via a Pull Request by you or another community member. We thank you in advance for your contribution and are looking forward to reviewing it!
fc
commented
1 year ago removed (follow along with what Ben has, he's more onto the trail)..
benatshippabo
commented
1 year ago @sapphi-red I am testing what you suggested below. Now when switching between branches I get the error below.
SomeFile.tsxexists in the first branch but does not exist in the branch I'm switching to. I don't have a ton of time to dig into this so if someone feels inspired to take a look, feel free.
Hi @fc I made a pr #12125 that uses a hash to compare duplicated modules. Let me know if that fixes it
fc
commented
1 year ago Hi @fc I made a pr #12125 that uses a hash to compare duplicated modules. Let me know if that fixes it
You the man! I'm on the road a bit and I think my local dev is messed up so will try again later today/tomorrow.
fc
commented
1 year ago I made a mistake in my earlier testing so you can ignore my comment above yours. I re-produced the original bug first to make sure I was following the right steps... then, when testing a build of your branch it now just hangs and it's been running for several minutes with no updates in the UI or output in the terminal.
Then I went through the steps again and enabled DEBUG=vite:* but it didn't output anything useful in the terminal. It says "hmr update" then just hangs/stops - maybe it's stuck in some kind of infinite or recursive loop which hints at something else going on.
Looks like I need to drop in some console.log statements to see where it is getting stuck..
Maybe this is related to the issue you reported -- https://github.com/vitejs/vite/issues/12087
benatshippabo
commented
1 year ago Yeah I have this feeling it may be a cascade of errors for us too. Fixing one reveals another 🥲
fc
commented
1 year ago I put in some log statements to see what files it is trying to run at. At a glance, it looks like propagateUpdate is getting called over and over again with the same/similar group of files over and over gain. A circular dependency issue?..
Using your branch, in my logs it is basically hitting this call over-and-over again: https://github.com/vitejs/vite/blob/167753d3754507430600a1bc2b100ca321b17a86/packages/vite/src/node/server/hmr.ts#L313
I re-read through the stack trace and I was actually missing in my stack trace walkthrough/explanation that it was calling that line above for 20-ish times before it did the final call to propagateUpdate and erroring out.
benatshippabo
commented
1 year ago You may be on to something @fc there's the block of code above that is supposed to return early in the event of the circular dependency:
But again, like for sets, this won't work if the module node is not the same reference.
Maybe it needs to be:
if (currentChain.some((node: ModuleNode) => node.id === importer.id)) {
// circular deps is considered dead end
return true
} Assuming it is a bug in propagateUpdate, you should hypothetically be able trigger the infinite loop by creating a specially crafted call to propagateUpdate in a unit test.
benatshippabo
commented
1 year ago Assuming it is a bug in
propagateUpdate, you should hypothetically be able trigger the infinite loop by creating a specially crafted call topropagateUpdatein a unit test.
@fc okay I just pushed some updates to my branch implementing what I proposed earlier. Let's see if that helps
fc
commented
1 year ago @benatshippabo I did a test but it still looks like some kind of infinite loop. I re-added my logs and it just showed that it was still calling this line a lot. I'm going to see if I can figure exactly what args/params are being initially sent to propagateUpdate as maybe it needs to be differently defined than what is in that unit test. Did your unit test go into an infinite loop or trigger the Set RangeError when you didn't have your changes added in?
Edit: Sweet, it's possible to set breakpoints in vite with NODE_OPTIONS=--inspect-brk and it's easier to inspect the full stack trace with Chrome. TBH, the actual problem isn't standing out to me yet. I want to see if I can find a way to repro with a unit test by copying the variables I can extract with the inspector.
benatshippabo
commented
1 year ago @fc you are right, the test isn't comprehensive enough. Could you also share the parameters being passed to propagateUpdate, maybe the parameters in my unit test isn't covering enough
fc
commented
1 year ago @benatshippabo I've been looking at this tonight. The data is just too massive and extremely nested when I look at it in the debugger. I'm not super sure how I can even copy the data out. If there are 1000s of importers and then one of those importers has 1000s of importers it will then call propagateUpdate. The hash set is 1000+. I wish I could find something more useful.
It would probably be helpful if I understood propagateUpdate better but TBH I'm pretty perplexed.
mattrunyon
commented
1 year ago I will add that propagateUpdate not being properly de-duped also causes HMR performance problems. In my repo, I can change single lines that triggers 20k+ updates which is just a bunch of copies of the same 14 files actually. This only seems to happen when I use resolve.alias to include src files from other modules in my monorepo. When I don't use resolve.alias, the update array balloons to 67k. Again just the same 14 files/boundaries over and over.
ArnaudBarre
commented
1 year ago This clearly need a repro showing how to get Vite graph module getting duplicated modules (or a least showing than updates is called more times than file change events)
mattrunyon
commented
1 year ago I can provide our repo with the issue if you don't mind a non-minimal repro
I placed some breakpoints in the HMR code and here's some observations
node.isSelfAccepting branch is the only place boundaries are added in our case. These are duplicates of the same handful of nodes. propagateUpdate is not called from the CSS check in this branchnode.importers are never executed. propagateUpdate is always called at the end of that loopacceptedHmrDeps is always empty. I added a size check to verify and it is always 0acceptedHmrExports is always falsyArnaudBarre
commented
1 year ago If you can provide a repo where git clone + (p)npm i + name of the file to edit to trigger the behaviour I can look at it. If running the app requires external services to setup this would be complex to look at it
mattrunyon
commented
1 year ago I made a branch that doesn't require any services to run. Here is the link https://github.com/mattrunyon/web-client-ui/tree/vite-hmr-demo
npm install
npm run start:app or cd packages/code-studio then npm start. The script from the root just uses lerna to execute start in packages/code-studio.
Then navigate to localhost:4000/styleguide
You can save packages/grid/src/Grid.tsx to trigger a bad state. I'm getting 5387 boundaries in the set, but only 17 unique. If you want to see something change, scroll down to the Grids section of the styleguide. Then in the render function of packages/grid/src/Grid.tsx, add style={{ border: '1px solid blue' }} to the canvas tag. For me it takes around 20-30s to reflect the change vs 1-2s if I de-dupe the boundaries set (didn't dig into why it was called so many times)
mattrunyon
commented
1 year ago One other thing I noticed is it might be related to index files which re-export many modules in a monorepo. So in my repo above, Grid.tsx is re-exported by packages/grid/index.ts and then many other points import something (not necessarily the Grid component) from the grid package. Grid is not self accepting.
Other cases which I thought may trigger it would be saving something like components/src/Button.tsx. That component is re-exported by an index file which is then consumed from other packages in the monorepo. But Button.tsx is self accepting so it returns quickly
ArnaudBarre
commented
1 year ago Oh yeah barrels exports is really bad for Vite HMR model. I will look at it but if you want 1-2s updates for every file in your app you need to remove this and have explicit import as much as possible. This is not always simple when import files from a shared library in a monorepo, but that's pretty cool HMR works across monorepo I think. If the button case doesn't happen more often, it may be because you're not following fast refresh rules enough (but you should get a warning pointing to https://github.com/vitejs/vite-plugin-react-swc#consistent-components-exports in that case)
mattrunyon
commented
1 year ago Noted. The barrel exports are useful for keeping our imports non-breaking since it can be consumed as import { Button } from '@deephaven/components' and if the file moves or is renamed we don't need alias files in place.
We have the eslint rule recommended by the react-swc plugin and AFAIK all of our files are valid under that rule. So usually we don't run into any invalidations.
Just another point I noticed is that class components are not self accepting but functional components are. I know functional components are better for HMR, but class components seem to work ok (even if they're just remounted and state is discarded). In our case we don't really have the resources to convert our class components to functional components and are fine with losing the state if the component is remounted.
In the propagateUpdates function, it seems there's. not anything preventing propagating from a node multiple times. Which is where the barrel files seem to suffer. Any reason to not check if a node has been propagated and stop calling the function? It looks somewhat like a DFS traversal of the graph, but right now nothing prevents a node from being traversed multiple times.
ArnaudBarre
commented
1 year ago You could setup something to have import { Button } from '@deephaven/components/Button' but it's a lot of setup for just improving HMR. But I think inside an app the benefits of removing barrels file is worth it.
Even without the state update, the file is a boundary so that's keep HMR quick and I understand that keeping state is not worth the rewrite.
Ok this makes sense. I'm checking it now but if that's that I can fix that
mattrunyon
commented
1 year ago Thanks. The propagateUpdates function is very quick on the server either way. It's the update list containing thousands of duplicates that makes the client slow. So the client plugin assumes that every item in the list needs to be updated.
But fixing propagateUpdates recrawling nodes should probably fix both the HMR issue and the issue on this ticket of overflowing the call stack with a bunch of changes. If that is indeed the case
benatshippabo
commented
1 year ago would this plugin help with the barrel export issue?
ArnaudBarre
commented
1 year ago This sounds a lot of additional tooling just to avoid writing explicit import in the first place. And most people often combine barrels export with export *
But maybe this is worth it for the UI lib in monorepos
sapphi-red
commented
1 year ago Closing this issue as I guess it's fixed. There's two reports that confirmed #12658 fixed their issues (https://github.com/vitejs/vite/pull/12658#issuecomment-1489631731, https://github.com/vitejs/vite/issues/12087#issuecomment-1502249690).
Describe the bug
In our case, by running vite in local dev and switching between our release branches with
git checkoutthen we see the shell/vite crash and the browser die. It seems like HMR is not able to keep up with so many files changing so quickly.Examination of the stack trace
It seems like since so many files change at the same time, it triggers this event handler many times: https://github.com/vitejs/vite/blob/8c87af7c03f5b989c8c86cf0c4efb0313c24af82/packages/vite/src/node/server/index.ts#L506-L511
The event handler is calling
handleFileAddUnlink: https://github.com/vitejs/vite/blob/167753d3754507430600a1bc2b100ca321b17a86/packages/vite/src/node/server/hmr.ts#L195Which subsequently calls
updateModules: https://github.com/vitejs/vite/blob/167753d3754507430600a1bc2b100ca321b17a86/packages/vite/src/node/server/hmr.ts#L204Calls
propagateUpdateat this line over-and-over again (10+ times according to stack trace): https://github.com/vitejs/vite/blob/167753d3754507430600a1bc2b100ca321b17a86/packages/vite/src/node/server/hmr.ts#L313Then calls
propagateUpdateone more time before hitting theRangeError: https://github.com/vitejs/vite/blob/167753d3754507430600a1bc2b100ca321b17a86/packages/vite/src/node/server/hmr.ts#L148Finally, the exception/crash is triggered at this point: https://github.com/vitejs/vite/blob/167753d3754507430600a1bc2b100ca321b17a86/packages/vite/src/node/server/hmr.ts#L246
Reproduction
not yet...
Steps to reproduce
System Info
Used Package Manager
yarn
Logs
I think the browser crash is possibly due to how HMR tries to do a full reload of the page but because the vite server crashed it shows Chrome as unavailable:
Possibly related stack trace that doesn't cause total crash
The stack trace below can happen but in an overlay and appears to be more recoverable.
When a lot of files change but not as many as when switching between our release branches, then it doesn't lead to a complete vite crash but it has this stack trace that displays in the vite overlay. I suspect that the stack trace below and the one above have a related cause (i.e., lots of files change):
Validations