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

feat(sync-service): Encode JSON encode on write rather than read #1485

Merged
merged 6 commits into from
Aug 7, 2024

Conversation

robacourt
Copy link
Contributor

@robacourt robacourt commented Aug 7, 2024

JSON encoding on write rather than read to reduce memory footprint. For example the memory use of an initial sync of a 35MB/200k row table can be reduced from 50MB to 25MB on the first initial sync (which includes the encoding and the writing to storage) and to 6MB on subsequent initial syncs (where we just read from storage and there's no encoding).

This change also allows some simplifications, so I have included them as refactoring in this PR:

  • Logic to do with the structure and how to create Log Items has been consolidated to a new module LogItems
  • The prepared_change intermediate state has been removed. The transformation Changes > list(prepared_change) > LogItems now is simply Changes > LogItems which makes the code more readable ( prepared_change was a 5 tuple rather than a nicely labeled map) and easier to reason about (since there's one less data structure to worry about)
  • The snapshot row intermediate state has a few less references, ideally I'd like to get encapsulate it into a single module, but for now it's mainly Shapes.Querying that creates it and LogItems.from_snapshot_row/4 that reads it.
  • Some logic duplicated in InMemoryStorage and CubDbStorage has been consolidated.

I've rebased this PR into 2 commits, the functional change and the refactoring, to aid with reviewing.

Copy link

netlify bot commented Aug 7, 2024

Deploy Preview for electric-next canceled.

Name Link
🔨 Latest commit 692fcff
🔍 Latest deploy log https://app.netlify.com/sites/electric-next/deploys/66b3424ce6582f0008e02f5e

Copy link
Member

@alco alco left a comment

Choose a reason for hiding this comment

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

I have reviewed the first commit.

Will take a separate look at the refactoring and get back to you.

Copy link
Member

@alco alco left a comment

Choose a reason for hiding this comment

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

Brilliant! 💎

packages/sync-service/lib/electric/log_items.ex Outdated Show resolved Hide resolved
@robacourt robacourt merged commit f809c8d into main Aug 7, 2024
20 checks passed
@robacourt robacourt deleted the rob/json-encode-on-write branch August 7, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants