Optimize some cell and value extraction read paths #6073

schlosna · 2022-06-07T01:23:32Z

Goals (and why):
While looking through some JFRs for services using AtlasDB, noticed several read paths that could be optimized to reduce allocations and improve method execution and CPU utilization.

==COMMIT_MSG==
Optimize some cell and value extraction read paths
==COMMIT_MSG==

Implementation Description (bullets):

Testing (What was existing testing like? What have you done to improve it?):

Concerns (what feedback would you like?):

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday):

…r extraction

changelog-app · 2022-06-07T01:23:35Z

Generate changelog in `changelog/@unreleased`

Type

Description

Optimize some cell and value extraction read paths

Check the box to generate changelog(s)

Generate changelog entry

schlosna · 2022-06-07T02:33:45Z

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/Cell.java

-            throw e;
+    private byte[] validateNameValid(byte[] name) {
+        if (isNameValid(name)) {
+            return name;


we want to keep this method small so that it gets JITed & inlined for the Cell.create happy path

schlosna · 2022-06-07T02:37:05Z

atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CellLoader.java

        for (Map.Entry<CassandraServer, List<Cell>> hostAndCells : hostsAndCells.entrySet()) {
-            if (log.isTraceEnabled()) {
+            if (isTraceEnabled) {


in general case I wouldn't normally pull log.isTraceEnabled() out to a local, but this popped in profile for relatively hot loop this will almost always be false, so pulled it out a local final boolean

yeah, that makes sense

Jolyon-S

Again, nice changes - appreciate the work. I'm a little concerned about the breaks in Cell, though.

Jolyon-S · 2022-06-08T10:54:36Z

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/Cell.java

-            hashCode = Arrays.hashCode(rowName) ^ Arrays.hashCode(columnName);
+            hashCode = 31 * Arrays.hashCode(rowName) + Arrays.hashCode(columnName);


Per the comment on CellReference:

/** * {@link Cell#hashCode()} implementation has a rather unfortunate case where it is always 0 if the row name and * the column name match. We did not want to change it to keep backwards compatibility, but we need a uniform * distribution here for all reasonable patterns. */

I appreciate that this hash code is crap, but I think we should not break this.

Ahh, will revert and add comment here & test to ensure we hold that back compact invariant

updated & added test coverage

Jolyon-S · 2022-06-08T10:54:46Z

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/Cell.java

+        return this.hashCode() == other.hashCode()
+                && Arrays.equals(rowName, other.rowName)
+                && Arrays.equals(columnName, other.columnName);


As below, I think it'd be safer to keep this the same.

Jolyon-S · 2022-06-08T10:55:47Z

atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CellLoader.java

        for (Map.Entry<CassandraServer, List<Cell>> hostAndCells : hostsAndCells.entrySet()) {
-            if (log.isTraceEnabled()) {
+            if (isTraceEnabled) {


yeah, that makes sense

Jolyon-S · 2022-06-08T10:58:12Z

atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CellLoader.java

@@ -120,9 +121,9 @@ void loadWithTs(
                    SafeArg.of("totalPartitions", totalPartitions));
        }

-        List<Callable<Void>> tasks = new ArrayList<>();
+        List<Callable<Void>> tasks = new ArrayList<>(hostsAndCells.size());


while this is probably a better estimate, we're still possibly going to exceed this probably (as each hostsAndCells element may add several elements). Still an improvement, so fine by me

Yeah, this isn't fully optimal but should be more reasonable estimate

Jolyon-S · 2022-06-08T10:59:05Z

atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CellLoader.java

+        List<List<Cell>> batches = batcher.partitionIntoBatches(cells, cassandraServer, tableRef);
+        List<Callable<Void>> tasks = new ArrayList<>(batches.size());
+        for (final List<Cell> partition : batches) {


This reverts commit 6192110.

This reverts commit 68260ef.

schlosna · 2022-06-08T14:28:12Z

atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CellLoader.java

                log.trace(
                        "Requesting {} cells from {} {}starting at timestamp {} on {}",
-                        SafeArg.of("cells", hostsAndCells.values().size()),
+                        SafeArg.of("cells", hostAndCells.getValue().size()),


this was a drive by fix as this seemed to log incorrect value

yep - thanks!

Jolyon-S

Awesome, thanks! I really like all the things tested for Cell; I've got a few nits on that test class, but going to approve. Feel free to merge once those are resolved!

Jolyon-S · 2022-06-08T14:46:59Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CellTest.java

+        assertThat(Cell.create(bytes("row"), bytes("col")))
+                .isNotEqualByComparingTo(Cell.create(bytes("row2"), bytes("col")));
+        assertThat(Cell.create(bytes("row"), bytes("col")))
+                .isNotEqualByComparingTo(Cell.create(bytes("row2"), bytes("col2")));


I think you're missing the case where row is the same and column is different

Jolyon-S · 2022-06-08T14:48:10Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CellTest.java

+        assertThat(Cell.create(bytes("row1"), bytes("col"))).isLessThan(Cell.create(bytes("row2"), bytes("col")));
+        assertThat(Cell.create(bytes("row1"), bytes("col"))).isGreaterThan(Cell.create(bytes("row0"), bytes("col")));


Also, the compareTo method has two branches, and this only tests the first (where the row differs); perhaps worth testing the compareTo part when row is the same and col is less or greater?

Jolyon-S · 2022-06-08T14:48:40Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CellTest.java

+    private static byte[] bytes(String value) {
+        return value.getBytes(StandardCharsets.UTF_8);
+    }
+}


nit: newline

spotless is failing on this

Jolyon-S · 2022-06-08T14:49:13Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CellTest.java

+        assertThat(Cell.create(bytes("row"), bytes("col")))
+                .describedAs("Cell unfortunately has a non-ideal hashCode where swapped "
+                        + "row and column values lead to the same hashCode and cannot be changed due "
+                        + "to backward compatibility. See goodHash")


nit: See CellReference's goodHash or CellReferenhce#goodHash

Jolyon-S · 2022-06-08T14:49:47Z

atlasdb-api/src/test/java/com/palantir/atlasdb/keyvalue/api/CellTest.java

+public final class CellTest {
+
+    @Test
+    public void create() {


nit: can we prefix create, isNameValid, and compareTo with test?

Jolyon-S · 2022-06-08T14:52:19Z

atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CellLoader.java

                log.trace(
                        "Requesting {} cells from {} {}starting at timestamp {} on {}",
-                        SafeArg.of("cells", hostsAndCells.values().size()),
+                        SafeArg.of("cells", hostAndCells.getValue().size()),


yep - thanks!

schlosna added 6 commits June 6, 2022 21:08

Optimize Cell name validation happy path

bcd8fce

Spread Cell hashCode

6192110

Cell equals compares hashCode first

68260ef

CellLoader does not use log.isTraceEnabled() in hot loop

e02d713

Presize CellLoader batch lists

cbb123e

ValueExtractor uses computeIfAbsent to accesses collector map once pe…

08e372b

…r extraction

svc-changelog and others added 2 commits June 7, 2022 01:23

Add generated changelog entries

c335fc1

Fix CellLoader::loadWithTs trace logging cells size

fc5046d

schlosna commented Jun 7, 2022

View reviewed changes

schlosna requested a review from jeremyk-91 June 7, 2022 21:06

Jolyon-S self-requested a review June 8, 2022 10:46

Jolyon-S suggested changes Jun 8, 2022

View reviewed changes

schlosna added 4 commits June 8, 2022 07:56

Revert "Spread Cell hashCode"

eff7daa

This reverts commit 6192110.

Revert "Cell equals compares hashCode first"

60cfb11

This reverts commit 68260ef.

Clarify non-ideal Cell#hashCode() implementation

76e8651

Add Cell unit test coverage

80f9d98

schlosna commented Jun 8, 2022

View reviewed changes

schlosna requested a review from Jolyon-S June 8, 2022 14:29

Jolyon-S approved these changes Jun 8, 2022

View reviewed changes

schlosna added 4 commits June 8, 2022 15:46

spotless

a324cae

address comments

0f3ad73

Cell test coverage

28e9966

Clarify CellReference#hashCode

1aa92eb

schlosna added the merge when ready label Jun 8, 2022

bulldozer-bot bot merged commit edbf024 into develop Jun 8, 2022

bulldozer-bot bot deleted the ds/perf branch June 8, 2022 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize some cell and value extraction read paths #6073

Optimize some cell and value extraction read paths #6073

schlosna commented Jun 7, 2022

changelog-app bot commented Jun 7, 2022 •

edited by schlosna

Loading

schlosna Jun 7, 2022

schlosna Jun 7, 2022

Jolyon-S Jun 8, 2022

Jolyon-S left a comment

Jolyon-S Jun 8, 2022

schlosna Jun 8, 2022

schlosna Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

schlosna Jun 8, 2022

Jolyon-S Jun 8, 2022

schlosna Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S left a comment

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

Jolyon-S Jun 8, 2022

		hashCode = Arrays.hashCode(rowName) ^ Arrays.hashCode(columnName);
		hashCode = 31 * Arrays.hashCode(rowName) + Arrays.hashCode(columnName);

		assertThat(Cell.create(bytes("row1"), bytes("col"))).isLessThan(Cell.create(bytes("row2"), bytes("col")));
		assertThat(Cell.create(bytes("row1"), bytes("col"))).isGreaterThan(Cell.create(bytes("row0"), bytes("col")));

Optimize some cell and value extraction read paths #6073

Optimize some cell and value extraction read paths #6073

Conversation

schlosna commented Jun 7, 2022

changelog-app bot commented Jun 7, 2022 • edited by schlosna 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

Jolyon-S 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

Jolyon-S 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

changelog-app bot commented Jun 7, 2022 •

edited by schlosna

Loading

Generate changelog in `changelog/@unreleased`