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

datastore: disable compaction (fixes 2x memory issue) #1535

Merged
merged 4 commits into from
Mar 9, 2023

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Mar 8, 2023

Disables bucket-level compaction aka. archiving until we support batching, thereby fixing the 2x memory issue. See below for rationale.

This doesn't impact garbage collection nor performance in any noticeable way.

Before:

$ cargo r -p test_image_memory
Logged 100 2048x1024 RGBA images = 800 MiB
1.6 GiB RAM used

After:

$ cargo r -p test_image_memory
Logged 100 2048x1024 RGBA images = 800 MiB
831 MiB RAM used

Closes #1397


The original long form rationale:

Alright so I've continued digging down the LogMsg-less rabbit hole following our discussion, until I hit a dead-end: we have a few places in the code where we actually expect to be able to fetch any message (both control and data) by MsgId so that we can check whether they have been purged or straight up display them in the UI.

Similar to the issues we've had with garbage collection, this is yet another instance that shows the disconnect between the viewer, which drives most of its logic using MsgIds, and the datastore, which has absolutely no clue what a MsgId is (because upon insertion, a message is reduced to its atomic primitives (components) and spread across many different storages (per entity + per timeline) and buckets (per space + per time): there simply is no such thing as a message anymore).

Contrary to the GC situation though, I don't see any hack that could help us workaround the issue in this case (and there are already too many hacks being piled on tbh). The only solution I can think of would be to make MsgIds actual first-class citizens in the store (i.e. create dedicated bi-directional indices to map back and forth between data and MsgIds), which coincidentally would fix all the issues with GC too since the underlying problem is the same: bridging the MsgId-driven viewer with the much lower-level datastore where such a construct is completely ambiguous atm.

Now, I don't think we should do that because A) that is definitely not trivial and B) it will all go to waste as soon we implement batching because at this point MsgIds will become mostly meaningless and we're gonna have to rethink how we do things in the viewer anyway.

So what I propose is this: I'll disable bucket compaction (so-called "archives") the ugly way for now (it can be done without impacting GC), which will fix the double-memory issue, and punt on all of this until we have batching implemented and working.

See also this internal slack thread.


image

@teh-cmc teh-cmc added 🪳 bug Something isn't working ⛃ re_datastore affects the datastore itself labels Mar 8, 2023
Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

Should we consider handling this with a change to the default DataStoreConfig instead so that it could be enabled more easily for testing going forward?

// TODO(cmc): Compaction is disabled until we implement batching.
// See https://github.com/rerun-io/rerun/pull/1535 for rationale.
//
// This has no noticeable impact on importance.
Copy link
Member

Choose a reason for hiding this comment

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

Can we back up this claim with data?

Copy link
Member Author

Choose a reason for hiding this comment

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

added benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working ⛃ re_datastore affects the datastore itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn off store archiving to hotfix memory duplication issue
2 participants