[LW] Rely on locally written values instead of the cache #5871

Jolyon-S · 2022-01-21T17:08:52Z

Goals (and why):

==COMMIT_MSG==
The lock watch cache no longer attempts to cache writes - instead, these will read from the maps tracked within the transaction (with similar performance but more consistent behaviour around concurrent writes).
==COMMIT_MSG==

Implementation Description (bullets):

Writes in the TCVSI now just block cached reads, and force "remote" reads (realistically, these are local to the transaction).
Modifying local updates now uses putIfAbsent for non-writes.

Testing (What was existing testing like? What have you done to improve it?):
Modified current tests, and added a few test cases in various places

Concerns (what feedback would you like?):
Are there still race conditions that we should worry about?

Where should we start reviewing?:
TCVSI

Priority (whenever / two weeks / yesterday):
ASAP - we want to get this rolling.

changelog-app · 2022-01-21T17:08:57Z

Generate changelog in `changelog/@unreleased`

Type

Description

The lock watch cache no longer attempts to cache writes - instead, these will read from the maps tracked within the transaction (with similar performance but more consistent behaviour around concurrent writes).

Check the box to generate changelog(s)

Generate changelog entry

…into jshah/lock-watch-glory

Jolyon-S · 2022-01-24T13:09:52Z

...shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/TransactionScopedCacheImpl.java

@@ -63,15 +63,13 @@ static TransactionScopedCache create(ValueCacheSnapshot snapshot, CacheMetrics m
    @Override
    public synchronized void write(TableReference tableReference, Map<Cell, byte[]> values) {


I could make this take Set<Cell>; I've left it as-is as that's nicer for the callers, but maybe we should just change that anyway.

Jolyon-S · 2022-01-24T13:10:17Z

...rc/test/java/com/palantir/atlasdb/keyvalue/api/cache/LockWatchValueScopingCacheImplTest.java

        eventCache.processStartTransactionsUpdate(ImmutableSet.of(TIMESTAMP_2), LOCK_WATCH_LOCK_SUCCESS);
+        valueCache.processStartTransactions(ImmutableSet.of(TIMESTAMP_2));


the absence of this line meant that cache 2 was always a no op cache (oops)

Jolyon-S · 2022-01-24T13:23:47Z

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java

        getCache().delete(tableRef, cells);
+        putInternal(tableRef, Cells.constantValueMap(cells, PtBytes.EMPTY_BYTE_ARRAY));


Without this change, there'd still be a race condition:

Thread 1:
-> put internal
-> pause (say GC)

Thread 2 (after put internal from above):
-> read from cache

this would of course fail validation. However, swapping them is safe, as it just invalidates; at the absolute worst this means that we'd read from the remote, so preserves correctness (but in practice this is likely extremely rare).

If we swap, and a thread barges in between, we end up seeing in the cache that there has been a local write, but when we try to read it we instead read remote (since it's not cached in the transaction yet). This is ok because we can just pretend that the read happened before the write in this case?
The problem with the old impl is that we would potentially update the cache claiming we read this write remotely because we get confused?

I believe the problem with the old impl is that we could update the KVS before the cache, and the read in the middle would fail validation. By swapping the order and changing the write semantics, we guarantee that we either are forced to read remotely or read the locally written value, so it's correct in both cases.

gmaretic

This looks correct, though I have a few questions to validate that I'm following correctly

...ed/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/TransactionCacheValueStoreImpl.java

gmaretic · 2022-01-25T12:13:09Z

...ed/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/TransactionCacheValueStoreImpl.java


        // Read values from the snapshot. For the hits, mark as hit in the local map.
        Map<Cell, CacheValue> snapshotCachedValues = getSnapshotValues(table, remainingCells);
-        snapshotCachedValues.forEach(
-                (cell, value) -> localUpdates.put(CellReference.of(table, cell), LocalCacheEntry.hit(value)));
+        snapshotCachedValues.forEach((cell, value) -> cacheHitInternal(table, cell, value));


This is the only functional change in this method, right? Reading writes and reads from the cache separately does not seem to do anything special

The only change is just that we're filtering out the writes entirely (because filtering them out from localUpdates isn't enough). The cacheHitInternal is just an extra

...ed/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/TransactionCacheValueStoreImpl.java

gmaretic · 2022-01-25T12:30:21Z

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java

        getCache().delete(tableRef, cells);
+        putInternal(tableRef, Cells.constantValueMap(cells, PtBytes.EMPTY_BYTE_ARRAY));


If we swap, and a thread barges in between, we end up seeing in the cache that there has been a local write, but when we try to read it we instead read remote (since it's not cached in the transaction yet). This is ok because we can just pretend that the read happened before the write in this case?
The problem with the old impl is that we would potentially update the cache claiming we read this write remotely because we get confused?

Jolyon-S · 2022-01-25T13:10:40Z

This looks correct, though I have a few questions to validate that I'm following correctly

You do raise an interesting point: I think we're actually going to have more misses with this approach. Specifically, the read-only cache created will not have local writes, but also won't have any cached values, and so will have to hit the KVS for those. What's stupid is that we filter those out anyway, so really we should just optimise the SerializableTransaction to not read them in the first place (especially since they're actually the wrong values!)

gmaretic

Ok thi smakes sense, though yes on the point above -- that is not ins the scope of this PR necessarily, but we should probably do it in a follow-up when you have time

svc-autorelease · 2022-01-25T14:36:36Z

Released 0.527.0

part 1

c962304

svc-changelog and others added 4 commits January 21, 2022 17:09

Add generated changelog entries

2f51eaa

fix some tests

7cdee9d

Merge branch 'jshah/lock-watch-glory' of github.com:palantir/atlasdb …

835450b

…into jshah/lock-watch-glory

subtle

aa145f4

Jolyon-S commented Jan 24, 2022

View reviewed changes

sneak

732b0d7

Jolyon-S changed the title ~~[WIP] [LW] Rely on locally written values instead of the cache~~ [LW] Rely on locally written values instead of the cache Jan 24, 2022

move write and delete to be earlier

1cf821c

Jolyon-S commented Jan 24, 2022

View reviewed changes

gmaretic reviewed Jan 25, 2022

View reviewed changes

CR

41f6282

gmaretic self-requested a review January 25, 2022 14:30

gmaretic approved these changes Jan 25, 2022

View reviewed changes

Jolyon-S added autorelease merge when ready labels Jan 25, 2022

bulldozer-bot bot merged commit 7906838 into develop Jan 25, 2022

bulldozer-bot bot deleted the jshah/lock-watch-glory branch January 25, 2022 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LW] Rely on locally written values instead of the cache #5871

[LW] Rely on locally written values instead of the cache #5871

Jolyon-S commented Jan 21, 2022 •

edited

Loading

changelog-app bot commented Jan 21, 2022 •

edited by Jolyon-S

Loading

Jolyon-S Jan 24, 2022

Jolyon-S Jan 24, 2022

Jolyon-S Jan 24, 2022

gmaretic Jan 25, 2022

Jolyon-S Jan 25, 2022

gmaretic left a comment

gmaretic Jan 25, 2022

Jolyon-S Jan 25, 2022

gmaretic Jan 25, 2022

Jolyon-S commented Jan 25, 2022

gmaretic left a comment

svc-autorelease commented Jan 25, 2022

		@@ -63,15 +63,13 @@ static TransactionScopedCache create(ValueCacheSnapshot snapshot, CacheMetrics m
		@Override
		public synchronized void write(TableReference tableReference, Map<Cell, byte[]> values) {

		eventCache.processStartTransactionsUpdate(ImmutableSet.of(TIMESTAMP_2), LOCK_WATCH_LOCK_SUCCESS);
		valueCache.processStartTransactions(ImmutableSet.of(TIMESTAMP_2));

		getCache().delete(tableRef, cells);
		putInternal(tableRef, Cells.constantValueMap(cells, PtBytes.EMPTY_BYTE_ARRAY));

[LW] Rely on locally written values instead of the cache #5871

[LW] Rely on locally written values instead of the cache #5871

Conversation

Jolyon-S commented Jan 21, 2022 • edited Loading

changelog-app bot commented Jan 21, 2022 • edited by Jolyon-S Loading

Generate changelog in changelog/@unreleased

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

gmaretic 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

Jolyon-S commented Jan 25, 2022

gmaretic left a comment

Choose a reason for hiding this comment

svc-autorelease commented Jan 25, 2022

Jolyon-S commented Jan 21, 2022 •

edited

Loading

changelog-app bot commented Jan 21, 2022 •

edited by Jolyon-S

Loading

Generate changelog in `changelog/@unreleased`