-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix out of memory building shaders #12166
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
As mentioned in the issue: I also noticed this, but didn't pinpoint the exact workflow that caused it. I now tried to reproduce it systematically. But that turns out to be a bit difficult. Issue detailsThe basic workflow to reproduce the issue:
The last two steps are the important ones: It seems like not all kinds of modifications cause the error. A somewhat reliable way of enforcing "errors" was to insert....
at the end of Sometimes it works. But in doubt, one can modify that line (e.g. change the 'red' component of the color) once or twice. Eventually, it should cause ~"an error" during the build. This error will announce itself by the growing memory usage. And eventually, it will
The out-of-memory error is one of the scary looking ones:
But for me, more frequently, it ended with this one
What's particularly annoying: It seems to leave ~"some of the build output in a messed-up state". Just trying to restart will then cause a
So a fresh "Review"Using the state of this PR, after modifying the
Doing a fresh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I did not (yet) review the code changes (because I'll have to read that more thoroughly, and don't really know the build code even before this change), but ... based on a test, as described in the comment, it seems like there's something wrong...)
I believe these are both due to the root cause I sorta highlighted in the original description. The read operations across the ~100+ shader files we have can take around 700ms on my system after I did some performance logging. These are currently done async so they can happen in different orders. ON TOP of that multiple "build calls" to
I can observe this error now too but don't see it in main either. I think this might be something I missed/misunderstood about the final build steps and I might be doing an early return that we can't do or missed steps that I didn't think were necessary. I'll take another look Edit: Ok I figured out what is going on. When we run |
@javagl this should be fixed now. The builtins were getting regenerated but empty with my first change. I've now switched to always generate the full file regardless if we've already processed it in builting. This has the benefits of still only needing to build |
The symptoms and error message suggested some concurrency issue - roughly: Trying to open/modify a file that is already opened. Whether or not that is caused by the memory issue, or an unrelated issue, is hard to say. I took care to only modify one file, and wait for the build to complete, but when the build itself is accessing multiple files concurrently, this may not avoid it. (One "stress test" that I indended to do - once this PR avoided the memory issue - was to quickly edit a file, hit "reload", edit and save the file, and hit reload again. Well, how else to ~"provoke/test/debug" concurrency issues....? If this causes some access error (independent of the memory issue), it can probably be tracked separately). I'll try out that state early tomorrow. |
Just recording some offline conversation with @ggetz. It's possible the better change here would be to actually load the |
Sorry for the delay. (I've been caught up in another PR, where this build issue has actually bugged me for a while - so it's good when this is about to be fixed). With the current state, it does not cause any delays or runaway memory usage for me. But it also doesn't seem to pick up the changes in the shader file. The workflow that I just tried:
When shutting down and restarting, and then hitting 'Run (F8)', it does pick up the change. |
To elaborate on this, I think what we'd be able to do is pass a loader option to esbuild in order to load However, this won't account for built-ins or for the custom GLSL "minification" which we do during release. One option would be to create a resolve plugin to handle these cases. |
@jjspace Are there plans to move forward with this soon? Or should we close for the time-being? |
The last commit before Prettier v3 was used to reformat the code
This commit reformats all code using prettier v3 relative to the pre-prettier-v3 tag
@jjspace I'm going to close for the time being. Feel free to re-open once this is ready for review again! |
Description
Our
glslToJavascript
build function had some outstanding issues that lead to huge spikes in memory usage when running the local development server. After some performance digging I made a couple small but very impactful changeswriteFile
always updated it regardless if it changed. This caused every file to need to be rebuilt. Now it checks firstglslToJavascript
function to take specific files to look at. Guarantees it runs super fast on a single file when one changes and no longer bottlenecksIssue number and link
Fixes #12164
Testing plan
npm start
for the local serverAuthor checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change