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

MessageLintReports get() and getAll() async, with no reactivity, to reduce excessive resource consumption #2378

Merged
merged 17 commits into from
Apr 5, 2024

Conversation

jldec
Copy link
Contributor

@jldec jldec commented Mar 12, 2024

partial fix for #2327 (memory consumption still limits scale)
resolves #2073

https://www.loom.com/share/3cf3c51d2f644886948c1267fa73f176

remove reactive map and make lint reports api async

MessageLintReportsQueryApi is simplified to make it non-reactive (no subscriptions), and the lint-report-specific ReactiveMap is replaced with a vanilla Map. (This change replaces the additional includedMessageIds api from the first experiment.)

Reasoning 💡
Lint reports don't require their own separate reactive map over all messages. It should be possible to maintain an up-to-date model of lint reports in memory, and build reactive (auto-updating) UIs with lint reports, both using only the messages reactive interface to watch for changes.

This would the first step toward removing granular (per message) reactivity from the sdk core.

status:

  • async lint reports get() and getAll()
  • remove lint reports reactivity
  • load-test changes
    • find scalability limit at 4GB node heap size:
      limit = ~10000 messages with translation into 36 languages
  • fix tests
  • code changes in fink and sherlock to test UX improvement at feature parity
    • fix off-by-one total lint errors value in fink
    • fix empty message list on reload with lint reports filter
  • changesets

first experiment - new api IncludedMessageIds (abandoned)

First attempt to fix the issue replaced subscription to getAll() with a subscription to includedMessageIds. This new api returns just the message IDs which have lint reports, instead of returning all lint reports in every event, making it much lighter. The change was implemented in d7ae5c1 but does not fully resolve the issue.

https://www.loom.com/share/358ae27b6dda403aa4c013a2ac0b723b
The loom show the out of memory failure when translating 4500 messages with
project.query.messageLintReports.includedMessageIds.subscribe in the load test.

Copy link

changeset-bot bot commented Mar 12, 2024

🦋 Changeset detected

Latest commit: b5fef9f

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

This PR includes changesets to release 26 packages
Name Type
@inlang/sdk Minor
@inlang/badge Patch
@inlang/cli Patch
@inlang/editor Patch
@inlang/github-lint-action Patch
vs-code-extension Patch
@inlang/sdk-load-test Patch
@inlang/rpc Patch
@inlang/settings-component Patch
@inlang/telemetry Patch
@inlang/website 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-js-adapter-unplugin Patch
@inlang/server Patch
@lix-js/server Patch
@inlang/paraglide-js-adapter-rollup Patch
@inlang/paraglide-js-adapter-vite Patch
@inlang/paraglide-js-adapter-webpack Patch
@inlang/paraglide-js-adapter-astro Patch
@inlang/paraglide-js-adapter-sveltekit Patch
@inlang/paraglide-js-adapter-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

@jldec jldec changed the title WIP instrument lint reports WIP instrument lint reports and experiment with lighter lint report api Mar 14, 2024
Copy link
Contributor

github-actions bot commented Mar 26, 2024

🥷 Ninja i18n – 🎉 Translations have been successfully updated

@jldec jldec changed the title WIP instrument lint reports and experiment with lighter lint report api Remove lint report reactivity to reduce excessive resource consumption Apr 1, 2024
Copy link
Member

@NiklasBuchfink NiklasBuchfink left a comment

Choose a reason for hiding this comment

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

I found a bug when loading the Fink with a lint rule filter selected:
https://www.loom.com/share/84bc0df455bf45f48435a6f2f6a38ac4

It is not something SDK-related and more about Finks application state, so let me know, if I should fix this.

package.json Outdated Show resolved Hide resolved
@jldec
Copy link
Contributor Author

jldec commented Apr 4, 2024

Copy link
Contributor

@felixhaeberle felixhaeberle left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@samuelstroschein samuelstroschein temporarily deployed to lint-light - badge-service PR #2378 April 4, 2024 19:01 — with Render Destroyed
@jldec
Copy link
Contributor Author

jldec commented Apr 5, 2024

It is not something SDK-related and more about Finks application state, so let me know, if I should fix this.

@NiklasBuchfink I have pushed a fix for this
(surprisingly) the signal produced by createResource only updates itself on the tick after the awaited fetch resolves, so I had to workaround this.

@samuelstroschein samuelstroschein temporarily deployed to lint-light - fink-editor PR #2378 April 5, 2024 11:53 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to lint-light - inlang-website PR #2378 April 5, 2024 11:53 — with Render Destroyed
@jldec jldec changed the title Remove lint report reactivity to reduce excessive resource consumption MessageLintReports get() and getAll() async, with no reactivity, to reduce excessive resource consumption Apr 5, 2024
@jldec jldec merged commit ea473bc into main Apr 5, 2024
6 checks passed
@jldec jldec deleted the lint-light branch April 5, 2024 12:08
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2024
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.

lint report query API should be async
4 participants