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

introduces sollid batching for loadMessagesViaPlugin #2821

Merged
merged 1 commit into from
May 29, 2024

Conversation

martin-lysk
Copy link
Contributor

@martin-lysk martin-lysk commented May 27, 2024

Previously we executed message updates in batches (500 updates per batch) and waited for the next tick to mitigate the mass production of cached triggeres in ReactiveMap.

We mitigate the problem of trigger production by batching all changes to the reactive map and reduce signal production to one per loadMessagesViaPlugin call instead.

This means getAll() and get() signals are produced once per message load instead of per message changed when messages loaded from a plugin.

Example results of load-test executed with pnpm run load-test 10000 1 1 1

ℹ Machine translated message "message_key_9993"                                             12:22:46 PM
ℹ Machine translated message "message_key_9994"                                             12:22:46 PM
ℹ Machine translated message "message_key_9995"                                             12:22:46 PM
ℹ Machine translated message "message_key_9996"                                             12:22:46 PM
ℹ Machine translated message "message_key_9997"                                             12:22:46 PM
ℹ Machine translated message "message_key_9998"                                             12:22:46 PM
ℹ Machine translated message "message_key_9999"                                             12:22:46 PM
ℹ Machine translated message "message_key_10000"                                            12:22:46 PM
✔ Machine translate complete.                                                               12:22:46 PM
  load-test messages getAll event: 2, length: 10000 +12s
  load-test lint reports changed event: 2, length: 0 +95ms
  load-test load-test done - exiting +8s

closes opral/inlang-message-sdk#72

Copy link

changeset-bot bot commented May 27, 2024

🦋 Changeset detected

Latest commit: 64e30ee

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

This PR includes changesets to release 30 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/paraglide-next-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 27, 2024 10:39
@martin-lysk
Copy link
Contributor Author

This PR should give the same result for the last signal produced by the SDK when multiple messages updated via the filesystem.

@LorisSigrist this should reduce the getAll() noise you are facing in paraglide

@felixhaeberle can you check vs code extensions reactivity based on getAll
@NiklasBuchfink please check fink - not sure how we can trigger multiple messages comming in via fs after project was initially loaded (pull maybe?)
@jldec I run the load-test successfully already . please check the code - happy to assist

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.

Fink never updates multiple messages at once atm. Even after a mergeUpstream or after a settings update we refetch the repo or project. That means we're good to go.

@felixhaeberle
Copy link
Contributor

Fink never updates multiple messages at once atm.

Sounds that this is the same with Sherlock, although this can change in the future.


👍 lgtm from Sherlock side 🕵️‍♂️

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.

LGTM
With this PR I could load and translate 10k messages - but crashed OOM with 20k.
Previously the number of getAll() events took a long time to resolve, but we didn't crash. I think the new model is better, so 👍 to merge this and help apps like paraglide.

(code was easier to look at with hide-whitespace enabled in github PR viewer)
Screenshot 2024-05-28 at 23 21 06

@martin-lysk
Copy link
Contributor Author

martin-lysk commented May 28, 2024

LGTM With this PR I could load and translate 10k messages - but crashed OOM with 20k. Previously the number of getAll() events took a long time to resolve, but we didn't crash.

Great - the memory consumption will be mitigated by https://linear.app/opral/issue/MESDK-108/requirements-for-lint-rule-scheduling. In the morning of 29.5.

@martin-lysk martin-lysk merged commit 4dc7833 into main May 29, 2024
3 checks passed
@martin-lysk martin-lysk deleted the martinlysk1/mesdk-111 branch May 29, 2024 10:04
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 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.

Limit query.messages.getAll() events when multiple messages change
5 participants