Skip to content
This repository was archived by the owner on Feb 13, 2025. It is now read-only.

Fix memory leak issue caused by reference cycle #27

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rouzwelt
Copy link

@rouzwelt rouzwelt commented Feb 4, 2025

Motivation

Atm there is memory leak issue, caused by cyclic references that MagicString and Chunk struct instances hold on to, to reproduce the memory leak issue we can just run the following code snippet and monitor the memory usage:

let text = "some text with few lines"
loop {
      let mut generator = MagicString::new(text);
      generator.prepend("some other text");
      generator
          .overwrite(
              6,
              7,
              "some overwrite text",
              OverwriteOptions::default(),
          );
      let res = generator.to_string();
      println!("{}", res);
      std::thread::sleep(std::time::Duration::from_millis(50));
}

Memory leak comes mostly from split() method of Chunk where a reference cycle gets created at very last couple lines of the method (at least as far as I've been able to detect) which causes instances of MagicString to not completely free up the memory after they go out of scope.

memory-leak

Proposed Solution

To fix and avoid memory leak issue we simply need to drop all the cyclic references that have been created during MagicString method calls when an instance of MagicString is going out of scope, so we add a new private method for Chunk to set its Option<Rc<RefCell>> to None which ensures that all cyclic reference go out of scope explicitly:

pub(crate) fn clear(&mut self) {
  self.next = None;
  self.prev = None;
}

next we just implement Drop trait for MagicString to call clear() for its Rc<RefCell<Chunk>> properties:

impl Drop for MagicString {
  fn drop(&mut self) {
    self.first_chunk.borrow_mut().clear();
    self.last_chunk.borrow_mut().clear();
    self.last_searched_chunk.borrow_mut().clear();
    self.chunk_by_end.iter_mut().for_each(|v| v.1.borrow_mut().clear());
    self.chunk_by_start.iter_mut().for_each(|v| v.1.borrow_mut().clear());
  }
}

this ensures that any cyclic reference that has been created during method calls and are persisting even after an instance of MagicString goes out of scope, gets dropped explicitly before dropping the whole MagicString instance.
To check if the fix works, we can just run the snippet above once again and monitor the memory usage.

PS: There are probably other approaches to address and fix this issue, but I found this approach to be very minimal, and it doesn't touch and change any other parts of the code and logic while still guaranteeing no memory leak, as the logic of this lib has many recursive components and changing it might possibly introduce unintended bugs.

@rouzwelt
Copy link
Author

rouzwelt commented Feb 4, 2025

@rouzwelt
Copy link
Author

rouzwelt commented Feb 12, 2025

@h-a-n-a @Brooooooklyn @faga295 would be nice to get a resolution for this, this is kinda critical issue with this lib, its been a week with no response, I was tbh planning on publishing a fork with the fix if this lib isnt being maintained. looking forward to your reply

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

Successfully merging this pull request may close these issues.

1 participant