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

use plugin rehype-highlight v7.0.0 memory leak #791

Closed
4 tasks done
zzzgydi opened this issue Nov 7, 2023 · 16 comments
Closed
4 tasks done

use plugin rehype-highlight v7.0.0 memory leak #791

zzzgydi opened this issue Nov 7, 2023 · 16 comments
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on

Comments

@zzzgydi
Copy link

zzzgydi commented Nov 7, 2023

Initial checklist

Affected packages and versions

rehype-highlight v7.0.0

Link to runnable example

https://github.com/zzzgydi/rhv7-memory-leak-test

Steps to reproduce

  • pnpm i
  • pnpm dev
  • Visit http://localhost:5173
  • Open Chrome's task manager and find the corresponding tab to observe the memory situation
  • Click start button
  • observe the memory situation
  • Click stop and clear button and observe

As a comparison

  • Change rehype-highlight v7.0.0 to rehype-highlight v6.0.0 in package.json
  • Repeat the steps above

Expected behavior

same as rehype-highlight v6.0.0

Actual behavior

image

Runtime

No response

Package manager

pnpm

OS

macOS

Build and bundle tools

Vite

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Nov 7, 2023
@immccn123
Copy link

immccn123 commented Nov 8, 2023

can reproduce in https://remarkjs.github.io/react-markdown/

just type freely in the editor and a large increase of memory will be found

@ChristianMurphy
Copy link
Member

Thanks @zzzgydi and @immccn123!
Memory should not be ballooning in that way.
Would either of you be interested in tracing this further and potentially opening a PR? https://developer.chrome.com/docs/devtools/memory-problems

@wooorm
Copy link
Member

wooorm commented Jan 5, 2024

I’m pretty sure V8 and friends actually use up as much memory as they can before freeing it? Maybe you’re seeing memory being used?

@zzzgydi
Copy link
Author

zzzgydi commented Jan 5, 2024

I guess that lowlight has registered languages internally, but did not unregisterLanguage. After creating multiple instances like this may cause memory leaks.

Below I have written three comparative examples. In my understanding, if there is no memory leak, the memory should be released after 100 iterations of the for loop.

import { createLowlight, common } from "lowlight"; // version: ^3.1.0
import HighlightJs from "highlight.js"; // version: ^11.9.0
import "./App.css";

function customCreateLowlight(grammars: typeof common) {
  const high = HighlightJs.newInstance();
  if (grammars) {
    Object.keys(grammars).forEach((name) =>
      high.registerLanguage(name, grammars[name])
    );
  }
  return high;
}

function App() {
  return (
    <div style={{ display: "flex", flexDirection: "column", gap: 12 }}>
      <button
        type="button"
        onClick={() => {
          for (let i = 0; i < 100; i++) {
            const lowlight = createLowlight(common);
          }
        }}
      >
        createLowlight
      </button>

      <button
        type="button"
        onClick={() => {
          for (let i = 0; i < 100; i++) {
            const lowlight = customCreateLowlight(common);
          }
        }}
      >
        customCreateLowlight
      </button>

      <div>
        <button
          type="button"
          onClick={() => {
            for (let i = 0; i < 100; i++) {
              const lowlight = customCreateLowlight(common);
              Object.keys(common).forEach((name) =>
                lowlight.unregisterLanguage(name)
              );
            }
          }}
        >
          customCreateLowlight & unregisterLanguage
        </button>
      </div>
    </div>
  );
}
export default App;

I obtained the following results. The results are divided into three rounds. First, I refresh the page, recorded, and then click on the button to run once, and recorded next.

image image

Through the above graph, it is a comparison of the results between snapshot 6 and snapshot 5. The largest occupation is string type, and these values are related to language settings within highlightjs.

@wooorm
Copy link
Member

wooorm commented Jan 9, 2024

After creating multiple instances like this may cause memory leaks.

If there is indeed a memory leak instead of chrome using up the memory it can use up, then that seems like a highlightjs issue?

It’s JavaScript; you’re not supposed to need to manage your own memory by removing registered things. The language does garbage collection for you.

@TonyL1u

This comment was marked as duplicate.

@wooorm

This comment was marked as resolved.

@TonyL1u
Copy link

TonyL1u commented Jan 10, 2024

The solution is you can contribute to open source and figure out a solution instead of spamming duplicate info.

good advice

@alielmorsy
Copy link

alielmorsy commented Feb 8, 2024

Did anyone find a solution for that?
I am using that code and yes there is a weird memory leak because it renders code blocks over and over:

   <Markdown rehypePlugins={[rehypeRaw, rehypeHighlight]} components={{
                code({node, className, children, ...props}) {
                    return <div className={className}>
                        {children}
                    </div>
                },
            }}>
                {post.content}
            </Markdown>

@remcohaszing
Copy link
Member

If possible, you should provide components with a stable identity to avoid rerenders.

function Code({ node, className, children, ...props }) {
  return (
    <div className={className}>
      {children}
    </div>
  )
}

export function Post({ post }) {
  return (
    <Markdown
      rehypePlugins={[rehypeRaw, rehypeHighlight]}
      components={{code: Code}}
    >
      {post.content}
    </Markdown>
  )
}

Also, if possible, you should provide props with a stable identity to reduce rerenders even further.

function Code({node, className, children, ...props}) {
  return (
    <div className={className}>
      {children}
    </div>
  )
}

const components = {
  code: Code
}

const rehypePlugins = [
  rehypeRaw,
  rehypeHighlight
]

export function Post({ post }) {
  return (
    <Markdown
      rehypePlugins={rehypePlugins}
      components={components}
    >
      {post.content}
    </Markdown>
  )
}

@aeharding
Copy link

This memory leak appears to happen by simply including rehype-remark, even without any actual highlighted content.

If possible, you should provide components with a stable identity to avoid rerenders.

This does not noticeably address the leak in my testing

As others suggest, my current workaround is to use v6.

@tgram-3D
Copy link

tgram-3D commented May 6, 2024

I'm not sure if anyone else has run into this, but one thing of note is if you downgrade to rehype-highlight 6.0.0 you get a TS error similar to this:

hashicorp/next-mdx-remote#86

The code works fine with no memory leak, and you can add @ts-expect-error to get rid of it as per the above thread, but I'm curious if there's a better solution.

@immccn123
Copy link

immccn123 commented Aug 1, 2024

This issue seems to be caused by the DOMContentLoaded event listener on the window not being removed in a timely manner. You can type anything on https://remarkjs.github.io/react-markdown/ and use getEventListeners(window).DOMContentLoaded in a Chromium-based browser to compare the length before and after.

rep_o9Lvk9Iu5T.mp4

window.addEventListener("DOMContentLoaded",...); appears only once in the minimized code.

Results before and after executing the following code:

getEventListeners(window).DOMContentLoaded.forEach(x => window.removeEventListener("DOMContentLoaded", x.listener))

image

Snapshot 1: Before typing
Snapshot 2: After typing
Snapshot 3: After executing the code

@wooorm
Copy link
Member

wooorm commented Aug 1, 2024

Thanks for investigating this and looking for a proper fix, Imken!

@wooorm
Copy link
Member

wooorm commented Sep 17, 2024

Thanks again, closed in highlightjs/highlight.js#4095!

@wooorm wooorm closed this as not planned Won't fix, can't repro, duplicate, stale Sep 17, 2024
@wooorm wooorm added the 👀 no/external This makes more sense somewhere else label Sep 17, 2024

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

9 participants