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

Ram usage increases until system freeze/crash #2

Open
sanmue opened this issue Sep 10, 2024 · 32 comments · Fixed by zed-industries/extensions#1610
Open

Ram usage increases until system freeze/crash #2

sanmue opened this issue Sep 10, 2024 · 32 comments · Fixed by zed-industries/extensions#1610

Comments

@sanmue
Copy link

sanmue commented Sep 10, 2024

Thanks for starting this extension.
There seems to be a memory issue at the moment, reported as this zed issue #17412 in combinations with this extension.

After some time ram usage increases until system freeze/crash.

config with recommended settings -> problem stays

Zed2.log

@3JIou-home
Copy link

same problem

@yodatak
Copy link

yodatak commented Sep 12, 2024

I opened a issue because im not sûre if its linked to how zed work or thé extensions
zed-industries/zed#17412

@kartikvashistha
Copy link
Owner

Thanks for raising this issue @sanmue and others, I'm a little swamped this week, so will look at this during the weekend and come back to you.

@kartikvashistha
Copy link
Owner

kartikvashistha commented Sep 12, 2024

Is anyone facing this issue on macOS as well?

@3JIou-home
Copy link

Is anyone facing this issue on macOS as well?

yes, me.

@kartikvashistha
Copy link
Owner

kartikvashistha commented Sep 22, 2024

Update: I have narrowed down the issue - it's occurring due to the tree-sitter configuration in the extension.

My current best guess is this: since we are re-using the yaml grammar for ansible files, it's somehow accidentally creating a memory leak because we are using the same parser name for two different languages.

As the tree-sitter configuration for this extension is using a different repo than what's being used by Zed for yaml, I tried switching to the same exact configuration for this extension in my local, but that unfortunately doesn't help either. As such, I think we might need help from upstream for this.

@notpeter
Copy link

@kartikvashistha There should not be an interaction between the two similar tree-sitter grammars. One small difference, we are using zed-industries/tree-sitter-yaml and the extension appears to be using tree-sitter-grammars/tree-sitter-yaml, both are forks of ikatyang/tree-sitter-yaml. Here's the diff -- we're a single commit ahead.

Can you share any additional details about what led you to believe this is caused by the tree-sitter grammar ruling out ansible-language-server, ansible-lint, etc?

Am I correct in stating that this has only been observed on Linux?

I wanted to point you in the direction of debug: open language server logs in the command palette as well. It's a little spartan, but you can see LSP RPC calls which might yield some additional info if the issue is with LSP not tree-sitter.

Do you have any steps for reproduction (ideally on an open source repo) beyond "hack on some ansible scripts until it crashes"?

@kartikvashistha
Copy link
Owner

Hey @notpeter, thanks for reaching out here! Apologies for the delay on my end.

As you rightfully observed, the Zed project and this extension are using two different versions of tree-sitter-yaml. As I mentioned in my last comment here, I tried using the very same configuration in this extension for tree-sitter locally, but that didn't help either :/

Can you share any additional details about what led you to believe this is caused by the tree-sitter grammar ruling out ansible-language-server, ansible-lint, etc?

Sure - I first started looking into the lsp and ansible-lint to ensure they weren't causing an issue. After finding no luck in that area, I moved onto tree sitter highlighting - when I disabled the highlighting provided by this extension locally, the issue vanished!

Am I correct in stating that this has only been observed on Linux?

It was first reported on Linux and I have been able to reproduce it on Linux. However, per a previous comment on this issue, it is observed in macOS as well.

Do you have any steps for reproduction (ideally on an open source repo) beyond "hack on some ansible scripts until it crashes"?

Note, this memory issue occurs only on filetypes detected as YAML, and not Ansible. We just need the Ansible extension installed (and perhaps initialised at least once on an ansible file/project) and then when we start working on YAML files (that are not detected as Ansible files), the memory issue kicks in when editing the YAML files. Basically, edit the file, make errors in the yaml file, save it and the memory issue kicks in.

Do you have any steps for reproduction (ideally on an open source repo) beyond "hack on some ansible scripts until it crashes"?

For testing, I used this as shared by @yodatak in the original issue reported under the Zed repo. You can start editing any YAML file here and the memory issue will kick in.

Per this issue, it looks like your suggestion to disable the yaml-language-server didn't improve things, so perhaps it lends more credence to the theory of something being dodgy on the yaml tree-sitter side of things?

Please lmk if I can help in any other way here. Thanks a bunch!

@kartikvashistha
Copy link
Owner

I'm wondering - if the issue is indeed with tree-sitter, should I push an update that temporarily disables highlighting? At least folks will be able to use the LSP without it crashing the editor...

@3JIou-home
Copy link

3JIou-home commented Oct 2, 2024

I'm wondering - if the issue is indeed with tree-sitter, should I push an update that temporarily disables highlighting? At least folks will be able to use the LSP without it crashing the editor...

I could use a release like that.

@linux-cultist
Copy link

linux-cultist commented Oct 7, 2024

I have the same problem on Linux. I don't even have to edit a file, just opening a big ansible repo and the editor freezes up competely when the ansible plugin is enabled. If it's not enabled, no problem.

I get the same freezing even I have no config for it in my settings. Just having it installed is enough to trigger the bug in a ansible repo.

Nothing special in the log as far as I can see.

Sometimes it even kills my entire Wayland session and the entire desktop is freezing until I reboot the entire machine, while the fan runs loud, so something is working hard doing something....

After reading this thread, I guess it could have to do with syntax highlighting but I couldn't tell. Anything you can do to try and fix this would be very appriciated since I would love to use zed for ansible.

@linux-cultist
Copy link

linux-cultist commented Oct 7, 2024

Just to show you what happens on my system, and I have tons of memory:

Before launching zed, over 40 GB memory free

image

Launching zed in a big ansible repo with the extension enabled - it takes less than 5 seconds for all 48 GB of memory to be maxed out and then zed crashes.

image

Maybe it helps to figure out whats wrong - it certainly seems like memory is not being released, but even so, why would the plugin use such an enormous amount of memory so very quickly? So its very interesting.

This is without any settings in zed, just the plugin installed.

@notpeter
Copy link

notpeter commented Oct 7, 2024

@linux-cultist What processes are using the memory? Is it Zed proper or is it a subprocess?

  • Linux: ps aux --sort=-%mem |head 100
  • Mac: ps -axm -o pid,rss,command | sort -nr -k 2 | head -n 20

If it's the ansible-language-server or the yaml-language-server misbheaving it's relatively easy to check, e.g. disable the yaml-language-server with following settings:

  "lsp": {
    "yaml-language-server": {
      "binary": {
        "path": "/fake"
      }
    }
  }

Note, these files being edited aren't really yaml, they are probably jinja2+yaml.

If this is actually the tree-sitter grammar I would love to find a reproduction test case. One bad grammar I think should only be limited to 4GB of ram (32 bit wasm) and Zed should be able to survive the wasm extension host crashing hanging.

@linux-cultist
Copy link

linux-cultist commented Oct 7, 2024

Hi and thanks for quick response!

This is the process started that eats all memory very quickly:

/usr/lib/zed/zed-editor zed-cli:///tmp/.tmpTWED1K/

Screenshot:

image

Will try to disable the yaml-language-server...

@linux-cultist
Copy link

linux-cultist commented Oct 7, 2024

With yaml-language-server disabled, it takes longer for zed to run out of memory. Probaby 20 seconds instead of 5. But it still does, eventually.

So its some kind of multiplication effect there between the plugins when its enabled, but its not causing the bug it seems.

@linux-cultist
Copy link

zed.log

Attaching zed.log so you can see details from one session before it crashes, maybe you see something strange in there.

@kartikvashistha
Copy link
Owner

I have created a test branch here that disables highlighting: https://github.com/kartikvashistha/zed-ansible/tree/hotfix/disable-highlighting

To test this, clone the repo, check out the above branch, and then install it via Install Dev Extension option under Extensions in Zed.

After disabling highlighting, I haven't seen any crashes when yaml files are opened. Please let us know here, if this is not the case on your machine.

Thanks!

@linux-cultist
Copy link

Yes, I can confirm that the hotfix without highlighting works very good, no increased memory usage and stable. It really is night and day.

So something happens with the highlighting that eats all memory in seconds. :)

@kartikvashistha
Copy link
Owner

I performed a few more tests locally:

  1. Disabled the Ansible lsp and enabled highlighting of Ansible files: Here, I ensured that my grammar repo and rev matched to what is used within Zed. Unfortunately, the current memory leak problem still persisted.

  2. Removed grammar section under extension.toml: When I was initially developing & publishing the extension, I hadn't explicitly defined the tree-sitter configuration under extensions.toml, which although worked just fine locally, didn't pass the CI when the PR for this extension was first raised. By removing this configuration, I can confirm that so far, I have been unable to reproduce the memory leak issue.

Regarding number 2, can someone check and confirm the same please? You can do so by pulling the latest changes in the repo, checking out the following commit (which resides in this branch) and installing the extension locally: git checkout 01a3f25cb769750b23514232909b23f7083b2624

I think the issue is specifically confined within tree-sitter-yaml - specifically, when we try to re-use this particular grammar with some other language(s). When I tried to reuse other non-yaml language grammars for some test extensions locally, I did not notice the memory issue.

Per my above findings and observations, I think that it could be more appropriate to track this issue in upstream rather than here as we have been able to sufficiently confirm that the Ansible LSP isn't the issue here - what do you think @notpeter ?

Thanks!

@notpeter
Copy link

Original upstream ikatyang/tree-sitter-yaml hasn't seen an update in 3yrs so I guess the zed fork zed-industries/tree-sitter-yaml is the logical place to be.

Does anyone have an open source repo which triggers this issue? I think that's the part I've been struggling with the most is that everyone seems have a private repo that is unusable and balloons Zed's memory to death -- I believe y'all, but I still haven't seen it for myself and am excited to try and track down the memory leak.

I wonder whether all this is just caused because tree-sitter-yaml has no business trying to parse yaml full of jinja2 escape sequences -- it's quite far from valid yaml. It's like trying to use an HTML parser to parse PHP code, you're probably going to have a bad time.

There appear to be multiple incomplete Jinja2 tree-sitters:

My cursory glance suggests that none of these is built for Ansible/SaltStack style YAML+Jinja2, but they might be a good prior art (please mind the GPL).

Conceivably one could modify tree-sitter-yaml to recognize:

{% ... %} for Statements
{{ ... }} for Expressions to print to the template output
{# ... #} for Comments not included in the template output

And then escape into the a distinct tree-sitter for the Jinja2 Python DSL.
Or at a first pass treat the escaped areas all as comments (first do no harm).
Or perhaps the opposite makes more sense (parse as jinja2 and then escape into yaml).
Not sure.

Assuming others validate the stability of your Ansible extension without tree-sitter-yaml, let's get a new version of that extension posted ASAP to keep more folks from getting caught up in this.

Thank you all for helping track this down, I appreciate your hard work.

maxdeviant added a commit to zed-industries/extensions that referenced this issue Oct 15, 2024
Currently, it is observed that the YAML TS parser, when resued within
the Ansible extension, is creating a memory leak when working with
non-ansible YAML files.

Since we aren't able to fully narrow down the issue on TS side just yet
to rollout the appropriate fix, I'm disabling highlighting temporarily
in order for at least the LSP be useful/accessible to the extension
users.

The issue can be tracked here:
kartikvashistha/zed-ansible#2

---------

Co-authored-by: Marshall Bowers <[email protected]>
@kartikvashistha
Copy link
Owner

Hey @notpeter ,

The only time I have seen the bug kick in is when we are editing YAML files, that aren't detected as an Ansible file. I haven't seen this issue (yet) for YAML files, that are detected as Ansible files.

Since Ansible files are typically in the .yml or .yaml format, my guess is that for a lot of folks encountering this issue in their Ansible repos is when their Ansible YAML files are not detected as an Ansible filetype.

Does anyone have an open source repo which triggers this issue? I think that's the part I've been struggling with the most is that everyone seems have a private repo that is unusable and balloons Zed's memory to death -- I believe y'all, but I still haven't seen it for myself and am excited to try and track down the memory leak.

If you need an Ansible repo to play around with, you can find some example code in this repo. Alternatively, you can use this repo of mine as well. To re-create the error, follow these steps broadly:

  1. Install the 0.0.1 version of the extension;
  2. Open an example ansible repo and/or ansible file once after installing the extension;
  3. Create or open an existing YAML file (that may or may not contain Ansible jinja2-syntax-like code within it) that is not detected as an Ansible file by Zed and try to edit the file such that you are making yaml indentation mistakes in order for the highlight to break. Saving the file continuosly after making such mistakes might also be required.
  4. The memory bug should kick in.

Note, for reference, I was using the following repo and commit as referenced by @yodatak in the original upstream issue to test and re-create the bug:

git clone [email protected]:yodatak/rapid-photo-downloader.git
cd rapid-photo-downloader
git checkout b386014e990c8dddd789192e0f31f4f3c86c76e7

Greatly appreciate your speedy responses here! 🙏🏽 Please lmk anytime if you have any other questions related to this. Thanks!

@kartikvashistha
Copy link
Owner

Also fyi, it looks the issue tab under Zed's tree-sitter-yaml repo is currently disabled, so I can't file an issue there.

@3JIou-home
Copy link

Greatly appreciate your speedy responses here! 🙏🏽 Please lmk anytime if you have any other questions related to this. Thanks!

tested for several days - did not find any problem

@kartikvashistha
Copy link
Owner

kartikvashistha commented Oct 28, 2024

Hi everyone,

I have got some good news - I think I have been able to resolve the highlighting and memory leak issues via a workaround. Can someone please help test this please? So far, I have not come across the infamous memory leak when highlighting is enabled via this fix.

Install the following commit via Install Dev Extension under Zed:

git clone [email protected]:kartikvashistha/zed-ansible.git
cd zed-ansible/
git checkout 7f4ebedacb68c520b902ed2a862b731962954d29

Thanks!

@linux-cultist
Copy link

Yes!

Tested the new commit and everything works, no memory leaks, syntax highlighting enabled.

So I think this can be pushed as official working version. :)

maxdeviant pushed a commit to zed-industries/extensions that referenced this issue Oct 30, 2024
This release enables back code highlighting for Ansible files to fix and
close [this](kartikvashistha/zed-ansible#2)
issue.

Other misc changes include:
- Bump `zed_extension_api` from `0.0.6` to `0.1.0`
- Update project README

PR: kartikvashistha/zed-ansible#5
@kartikvashistha
Copy link
Owner

I have shipped the proposed fix now to prod - if nobody reports anything worrying by the weekend, I'll close this issue.

Thanks everyone for your support in triaging the issue and testing my fixes along the way - I really, really appreciate it! 😃

@linux-cultist
Copy link

I noticed some more crashes with the plugin on my laptop today, but its not the same thing. Memory doesnt run out, but it makes zed crash.

But the fix above still makes the plugin usable, and now its some other bug that is causing the other crashes. Happens more infrequently too and not always.

I will create a new issue for it once I can sit down and find something that reproduces the problem. :)

@kartikvashistha
Copy link
Owner

Hey @linux-cultist - the only other 'infrequent' crash of the extension that I have encountered in the past is when the completion list returned by LSP is quite large (luckily you should be able to confirm this under Zed's logs when this occurs). I had made a note of this in the README.

Do lmk if this is what you see as well, thanks!

@kartikvashistha
Copy link
Owner

kartikvashistha commented Nov 5, 2024

Hey folks, another quick update:

After dogfooding the new release for a bit before closing this issue, I think I discovered another increased memory bug: basically, the memory starts to increase anytime a key-value pair is added at the very top of an Ansible file. This partially breaks highlighting and starts to create the increased memory + cpu issue.

I have been able to produce it on both macOS and Fedora 41. On macOS, there is a great cpu increase and memory keeps increasing gradually. On Fedora, the memory increases rapidly and OOM basically kills Zed.

Restarting the editor fixes both the highlighting (highlighting also gets resolved after closing and reopning the file) and resource usage as well. I have recorded a video showcasing the same: https://youtu.be/7Z--h9EeXeY

Honestly, I'm quite stumped on how to proceed next. My best guess is that there is some issue with WASM and Tree-sitter compilation/linking at the extension build stage as I noticed that if I re-use the existing yaml grammar from within the extension like so, then the issue dissapears completely (with a much lower observed resource usage). However, this fails the CI (as pointed here under (2)) which effectively means I cant ship it...

Unfortunately, I do not possess sufficient chops yet to further debug it effectively and as such would need help/guidance in helping to fix this. Fyi, both the extension and in built YAML configuration use the exact same grammar configuration, with the only difference being how they are sourced.

Maybe I can start with the following first question to @notpeter: is it possible to create an exception somehow to accept such a change? If this change is installed as a dev extension, then I do not see any weird YAML or memory issues.

Thanks!

@kartikvashistha
Copy link
Owner

kartikvashistha commented Dec 9, 2024

I have not been able to fix the last memory issue unfortunately - it seems it gets triggered anytime we try to add a yaml sequence or block just before a comment(i use vim motion shift + o when trying to insert above a comment) and/or the --- . Otherwise, it seems to be working fine.

I have also tried with both the Zed's yaml parser and an identical parser for Ansible (which is a clone of the YAML parser used by Zed for YAML but with a different name so as to avoid memory issues with normal non-ansible YAML files when the extension is installed), but was met with no success (PR).

The only way I can get it to work is via such a PR, but i'm unsure if we can submit such a change to Zed's extension repo (or allowed an exception).

Hey @notpeter - is it possible for a Zed member familiar with the YAML ts parser, Treesitter and the extension system to have a look here when they get some time (maybe @maxbrunsfeld) and give some pointers, if any? I have unfortunately hit a wall at the moment and would welcome any and all help here.

Thanks!

@artooro
Copy link

artooro commented Jan 29, 2025

How are you guys switching off syntax highlighting for the ansible file types? I would like to keep the linter but the memory issue is very annoying, and I'm not seeing a setting for highlighting specifically.
Just turned off validation.enable in the lsp settings to see if that will help at all.

@kartikvashistha
Copy link
Owner

Hey @artooro - validation.enable is an LSP setting, not a highlighting one.

To temporarily get around the memory issue, you can check out any of the following branches and install them manually as a Zed extension:

  1. With highlighting: https://github.com/kartikvashistha/zed-ansible/tree/re-use-yaml-grammar
  2. Without highlighting: https://github.com/kartikvashistha/zed-ansible/tree/hotfix/disable-highlighting

Since this memory issue isn't completely resolved, I'm thinking of reverting the highlighting changes and keeping it disabled till it gets fixed completely. I'll most likely do that on the weekend and open an issue in the main Zed repo with a report of what all I have discovered so far, as this issue needs to be tracked upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants