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

Fix solid memory leak #2738

Closed
wants to merge 4 commits into from
Closed

Fix solid memory leak #2738

wants to merge 4 commits into from

Conversation

martin-lysk
Copy link
Contributor

This change schedules message loading to the next tick on every 500th message to give solid a tick to free resources.

https://github.com/solidjs-community/solid-primitives/blob/9ca76a47ffa2172770e075a90695cf933da0ff48/packages/trigger/src/index.ts#L64

Copy link

changeset-bot bot commented May 8, 2024

🦋 Changeset detected

Latest commit: 29a7083

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 29 packages
Name Type
@inlang/sdk Patch
@inlang/badge Patch
@inlang/cli Patch
@inlang/doc-layout-component Patch
@inlang/editor Patch
@inlang/github-lint-action Patch
vs-code-extension Patch
@inlang/message-bundle-component Patch
@inlang/rpc Patch
@inlang/settings-component Patch
@inlang/telemetry Patch
@inlang/cross-sell-ninja Patch
@inlang/plugin-i18next Patch
@inlang/plugin-json Patch
@inlang/plugin-m-function-matcher Patch
@inlang/plugin-next-intl Patch
@inlang/plugin-t-function-matcher Patch
@inlang/paraglide-unplugin Patch
@inlang/paraglide-js-e2e Patch
@inlang/sdk-load-test Patch
@inlang/server Patch
next-js-testapp Patch
@inlang/sdk-multi-project-test Patch
@inlang/paraglide-rollup Patch
@inlang/paraglide-vite Patch
@inlang/paraglide-webpack Patch
@inlang/paraglide-astro Patch
@inlang/paraglide-sveltekit Patch
@inlang/paraglide-sveltekit-example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@martin-lysk martin-lysk requested a review from jldec May 8, 2024 15:43
@martin-lysk martin-lysk marked this pull request as ready for review May 8, 2024 15:49
Copy link
Contributor

@jldec jldec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment.

@@ -339,6 +344,7 @@ async function loadMessagesViaPlugin(
messages.set(loadedMessageClone.id, loadedMessageClone)
// NOTE could use hash instead of the whole object JSON to save memory...
messageState.messageLoadHash[loadedMessageClone.id] = importedEnecoded
loadedMessageCount++
Copy link
Contributor

@jldec jldec May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumping the count in 2 different code paths and testing for loadedMessageCount % messagesPerTick === 0 later, makes the code a bit fragile. E.g. what if 2 bumps happen and you miss the % x === 0.

In cases like this I think it's less fragile to test for > maxMessagesPerTick and then reset the count.

// move loading of the next messages to the next ticks to allow solid to cleanup resources
// solid needs some time to settle and clean up
// https://github.com/solidjs-community/solid-primitives/blob/9ca76a47ffa2172770e075a90695cf933da0ff48/packages/trigger/src/index.ts#L64
await 0
Copy link
Contributor

@jldec jldec May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer to use a setTimeout here. It's what we use for sleep in other places in the code like here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified that the behavior of sleep(0) with setTimeout(...,0) is not the same as await 0

@jldec
Copy link
Contributor

jldec commented May 10, 2024

Closing this PR - code is included in #2734

@jldec jldec closed this May 10, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2024
@jldec jldec deleted the fix-solid-memory-leak branch June 7, 2024 10:00
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.

2 participants