Performance regression when requesting a timestamp before the earliest version #2076

ognyanstoimenov · 2024-12-17T15:06:41Z

Reference Issues/PRs

What does this implement or fix?

Version Map entry now stores whether the whole chain has been loaded, so we don't need to reload again in certain scenarios

load_strategy_ is removed from version_map_entry and replaced with a flag is_earliest_version_loaded in LoadProgress as we only need to know whether the entire version chain has been loaded previously. Previously we only kept whether the cache has been loaded with an ALL strategy but that missed the cases where we load via DOWN_TO 0 or FROM_TIME that is before earliest
if we haven't loaded the entire version chain, we additionally check if we have loaded all undeleted versions. This is in cases where we have tombstone_all on a certain version and an undeleted version is requested. See Performance regression when requesting a timestamp before the earliest version #2076 (comment)
- this means that tombstone_all should not invalidate cache. see Performance regression when requesting a timestamp before the earliest version #2076 (comment)

Any other comments?

Checklist

Checklist for code changes...

Have you updated the relevant docstrings, documentation and copyright notice?
Is this contribution tested against all ArcticDB's features?
Do all exceptions introduced raise appropriate error messages?
Are API changes highlighted in the PR description?
Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

IvoDD

Main thing to change is the LOAD_ALL_UNDELETED regression + testing.

Others are some small refactors.

Also please update the PR description to describe better what are the core changes (i.e. the changes to the version map entry) and the behavior changes.

And I think you might need to rebase the PR to get the ASV fixes. I would really like to see the logs from ASV saying we have improved the benchmark from_epoch

IvoDD · 2024-12-20T09:24:14Z

cpp/arcticdb/version/version_map_entry.hpp

@@ -16,6 +16,8 @@
 #include <deque>
 #include <vector>

+#include "async/tasks.hpp"


I don't think we need this. Maybe remove?

Looks like we missed this?

IvoDD · 2024-12-20T09:26:16Z

cpp/arcticdb/version/version_map_entry.hpp

@@ -241,7 +244,6 @@ struct VersionMapEntry {
        tombstone_all_.reset();
        keys_.clear();
        load_progress_ = LoadProgress{};
-        load_strategy_ = LoadStrategy{LoadType::NOT_LOADED, LoadObjective::INCLUDE_DELETED};


It's nice that we remove the load_strategy from the entry. Could you explain in the PR description why it's now obsolete? Thanks!

IvoDD · 2024-12-20T09:40:29Z

cpp/arcticdb/version/test/test_version_map.cpp

+    const std::shared_ptr<VersionMap>& version_map,
+    const StreamId& id,
+    size_t number_of_versions,
+    std::vector<VersionId> tombstones_versions = {},


Nice, I like this refactor.

Currently this only allows us to specify a version chain where we only ever delete the latest version and can only have one tombstone all. It would be nice to also be able to test version chains like e.g.:

v0 <- v1 <- v2 <- t0 <- v3 <- tall2

To do this we can instead of the number_of_versions, tombstone_versions and tombstone_all_version have one argument:
std::vector<VersionChainOperation> operations
where an VersionChainOperation is just a struct of an enum {WRITE, TOMBSTONE, TOMBSTONE_ALL} and a version_id.

That's a good idea, will do it

IvoDD · 2024-12-20T09:51:56Z

cpp/arcticdb/version/test/test_version_map.cpp

@@ -604,7 +617,7 @@ TEST(VersionMap, FollowingVersionChain){
    auto store = std::make_shared<InMemoryStore>();
    auto version_map = std::make_shared<VersionMap>();
    StreamId id{"test"};
-    write_alternating_deleted_undeleted(store, version_map, id);
+    write_versions(store, version_map, id, 3, {2}, 0);


Looks like we use the write_alternating_deleted_undeleted in a few places.

To avoid chainging all these usages we can keep the write_alternating_deleted_undeleted and just define it as one call of the new write_versions.

I don't have a strong opinion but I think we should do one of:

delete write_alternating_deleted_undeleted

keep write_alternating_deleted_undeleted but make it use our new write_versions

Also maybe if we update to the slightly longer write_versions call it will be useful to keep around the write_two_versions to not have to write in many places {{Write, 0}, {Write, 1}}

IvoDD · 2024-12-20T09:54:06Z

cpp/arcticdb/version/test/test_version_map.cpp

    // FROM_TIME UNDELETED_ONLY should no longer be cached even though we used the same request before because the undeleted key it went to got deleted. So it will load the entire version chain
    check_loads_versions(LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(10)}, 3, 0);
+    // We have the full version chain loaded, so has_cached_entry should always return true (even when requesting timestamp before earliest version)
+    EXPECT_TRUE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(-1)}));
+    EXPECT_TRUE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::INCLUDE_DELETED, static_cast<timestamp>(-1)}));


Maybe one more assertion to showcase that it even works with e.g. LoadType::ALL?

Looks like we missed this one?
I mean something like:

EXPECT_TRUE(version_map->has_cached_entry(id, LoadStrategy{LoadType::ALL, LoadObjective::INCLUDE_DELETED}));

IvoDD · 2024-12-20T10:16:23Z

cpp/arcticdb/version/test/test_version_map.cpp

+        check_all_caching({load_all_undeleted_param}, should_load_to_v[i], i != 0);
+
+        // If we have loaded to version 0 all load requests are true
+        check_all_caching(should_load_to_v[i], {load_all_param, load_all_undeleted_param}, i == 0);


I think we can make check_all_caching(should_load_to_v[1], {load_all_undeleted_param}, true) pass?

It will need the comment about LOAD_ALL UNDELETED_ONLY caching changed to pass.

Actually even better, with your changes we can remove the separate testing for load_all and load_all_undeleted.

Whu don't just place the LOAD_ALL in should_load_to_v[0]
and LOAD_ALL_UNDELETED in should_load_to_v[1]?

IvoDD · 2024-12-20T10:24:42Z

cpp/arcticdb/version/version_map.hpp

@@ -606,14 +608,9 @@ class VersionMapImpl {
                return cached_timestamp <= requested_load_strategy.load_from_time_.value();
            }
            case LoadType::ALL:


Sorry I didn't think about this before but currently if we do: {LoadType::ALL, UNDELETED_ONLY} repeatedly it would never get cached.

That is because if we have the following version chain:

v0 <- v1 <- tall1 <- v2

the load_all_undeleted would reach the tall0 and terminate there.

I think we already have the information we need. I.e. if earliest_loaded_version <= tombstone_all.value() then we know that we have all undeleted versions.

It would also be good to test this.

Ah, good catch! Didn't think about that case

…v/performance_timestamp

IvoDD

Left a few more very minor comments. Otherwise all looks good! Thanks! Approved

IvoDD · 2024-12-23T17:06:52Z

cpp/arcticdb/version/test/test_version_map.cpp

+                break;
+            }
+            case VersionChainOperation::Type::TOMBSTONE_ALL: {
+                version_map->delete_all_versions(store, id);


Nit: Theoretically we can still have a tombstone_all to a version_id different than the last.
You can instead use the write_tombstone_all_internal + write_symbol_ref to do that.

If we decide against that it would be good to document that VersionChainOperation{TOMBSTONE_ALL, 3} might not do what we expect.

IvoDD · 2024-12-23T17:08:25Z

cpp/arcticdb/version/test/test_version_map.cpp

-    write_symbol_ref(store, key3, std::nullopt, entry->head_.value());
+    using Type = VersionChainOperation::Type;
+    write_versions(store, version_map, id, {{Type::WRITE, 0},
+                                                         {Type::TOMBSTONE_ALL},


Nit: These extra whitespaces seem off. Maybe align all the operations the same number of spaces to the right.

Hopefully @poodlewars's formatting will get merged and we can all use the same formatting throughout.

IvoDD · 2024-12-23T17:10:21Z

cpp/arcticdb/version/test/test_version_map.cpp

    // FROM_TIME UNDELETED_ONLY should no longer be cached even though we used the same request before because the undeleted key it went to got deleted. So it will load the entire version chain
    check_loads_versions(LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(10)}, 3, 0);
+    // We have the full version chain loaded, so has_cached_entry should always return true (even when requesting timestamp before earliest version)
+    EXPECT_TRUE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(-1)}));
+    EXPECT_TRUE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::INCLUDE_DELETED, static_cast<timestamp>(-1)}));


Looks like we missed this one?
I mean something like:

EXPECT_TRUE(version_map->has_cached_entry(id, LoadStrategy{LoadType::ALL, LoadObjective::INCLUDE_DELETED}));

IvoDD · 2024-12-23T17:11:11Z

cpp/arcticdb/version/test/test_version_map.cpp

+    using Type = VersionChainOperation::Type;
+    std::vector<VersionChainOperation> version_chain = {{Type::WRITE, 0},
+                         {Type::WRITE, 1},
+                            {Type::TOMBSTONE_ALL},


Nit: some more weird whitespaces

IvoDD · 2024-12-23T17:16:14Z

cpp/arcticdb/version/version_map_entry.hpp

@@ -16,6 +16,8 @@
 #include <deque>
 #include <vector>

+#include "async/tasks.hpp"


Looks like we missed this?

…v/performance_timestamp

ognyanstoimenov · 2025-01-06T09:51:31Z

cpp/arcticdb/version/test/test_version_map.cpp

-            ASSERT_FALSE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(1)}));
-            ASSERT_FALSE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(0)}));
-        }
+        // Tombstone All should not invalidate cache as it deletes everything so all undeleted versions have been loaded.


This is something I caught when testing a change related to this case #2076 (comment) . Previously writing tombstone_all would cause any subsequent UNDELETED_ONLY lookups in cache to return false. However, I think this was wrong because writing tombstone_all deletes all previous versions so it is impossible for an undeleted version to exist in the version chain and not in the cache. Thus all undeleted versions have been loaded in cache.

Also add tests that check_reload gives the correct result for these lookups?

Add tests where the tombstone_all version ID is not the latest version?

poodlewars · 2025-01-06T16:47:41Z

Please tidy up the git commit history

poodlewars

Add a benchmark to demonstrate the performance improvement?
Is this functionality covered by an end to end (Python) test?

poodlewars · 2025-01-06T16:59:30Z

cpp/arcticdb/version/test/test_version_map.cpp

-            ASSERT_FALSE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(1)}));
-            ASSERT_FALSE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(0)}));
-        }
+        // Tombstone All should not invalidate cache as it deletes everything so all undeleted versions have been loaded.


Also add tests that check_reload gives the correct result for these lookups?

poodlewars · 2025-01-06T17:03:09Z

cpp/arcticdb/version/test/test_version_map.cpp

-            ASSERT_FALSE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(1)}));
-            ASSERT_FALSE(version_map->has_cached_entry(id, LoadStrategy{LoadType::FROM_TIME, LoadObjective::UNDELETED_ONLY, static_cast<timestamp>(0)}));
-        }
+        // Tombstone All should not invalidate cache as it deletes everything so all undeleted versions have been loaded.


Add tests where the tombstone_all version ID is not the latest version?

ognyanstoimenov · 2025-01-07T09:18:06Z

Please tidy up the git commit history

Will squash before merging

Implementation

8944f2b

ognyanstoimenov requested review from alexowens90, willdealtry and poodlewars as code owners December 17, 2024 15:06

ognyanstoimenov added 2 commits December 19, 2024 14:06

Tests

7d7ec06

Fix tests

1951261

IvoDD requested changes Dec 20, 2024

View reviewed changes

ognyanstoimenov added 3 commits December 20, 2024 18:01

test

9bfa8f4

Merge branch 'master' of github.com:man-group/ArcticDB into ostoimeno…

d8f4f78

…v/performance_timestamp

PR comments

8a6a3de

IvoDD approved these changes Dec 23, 2024

View reviewed changes

ognyanstoimenov added 2 commits January 6, 2025 11:41

Tombstone All invalidation and formatting

3aa26ac

Merge branch 'master' of github.com:man-group/ArcticDB into ostoimeno…

0e6a1e3

…v/performance_timestamp

ognyanstoimenov commented Jan 6, 2025

View reviewed changes

poodlewars reviewed Jan 6, 2025

View reviewed changes

More testing

7b94b0c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance regression when requesting a timestamp before the earliest version #2076

Performance regression when requesting a timestamp before the earliest version #2076

ognyanstoimenov commented Dec 17, 2024 •

edited

Loading

IvoDD left a comment

IvoDD Dec 20, 2024

IvoDD Dec 23, 2024

IvoDD Dec 20, 2024

IvoDD Dec 20, 2024

ognyanstoimenov Dec 20, 2024

IvoDD Dec 20, 2024

IvoDD Dec 20, 2024

IvoDD Dec 20, 2024

IvoDD Dec 23, 2024

IvoDD Dec 20, 2024

IvoDD Dec 20, 2024

IvoDD Dec 20, 2024

ognyanstoimenov Dec 20, 2024

IvoDD left a comment

IvoDD Dec 23, 2024

IvoDD Dec 23, 2024

IvoDD Dec 23, 2024

IvoDD Dec 23, 2024

IvoDD Dec 23, 2024

ognyanstoimenov Jan 6, 2025 •

edited

Loading

poodlewars Jan 6, 2025

poodlewars Jan 6, 2025

poodlewars commented Jan 6, 2025

poodlewars left a comment

poodlewars Jan 6, 2025

poodlewars Jan 6, 2025

ognyanstoimenov commented Jan 7, 2025

Performance regression when requesting a timestamp before the earliest version #2076

Are you sure you want to change the base?

Performance regression when requesting a timestamp before the earliest version #2076

Conversation

ognyanstoimenov commented Dec 17, 2024 • edited Loading

Reference Issues/PRs

What does this implement or fix?

Any other comments?

Checklist

IvoDD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvoDD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ognyanstoimenov Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poodlewars commented Jan 6, 2025

poodlewars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ognyanstoimenov commented Jan 7, 2025

ognyanstoimenov commented Dec 17, 2024 •

edited

Loading

ognyanstoimenov Jan 6, 2025 •

edited

Loading