Speed up CompressedXContent.equals #79600

original-brownbear · 2021-10-21T07:14:30Z

The hash code on this one is stable (we compute it from the uncompressed bytes). It's essentially
free to compare it and early-break out in the false case.
In case of equals actually working though there are cases where compressed and uncompressed bytes
differ for the same content which made for a slow and potentially very allocation heavy comparison.
This commit (at the cost of some complexity) makes the equality checks needed to deduplicate Beats
style metadata about twice as fast in isolation and more importantly saves a massive amount of allocations
in them which should make for a larger practical speedup.
This has not been a huge deal in practice yet, but I would like to use the functionality to implement
metadata deduplication in a follow-up that is fairly simple but requires that the equals check in these
objects is safe to run in a hot loop on the master thread.

Relates #77466

The hash code on this one is stable (we compute it from the uncompressed bytes). It's essentially free to compare it and early-break out in the false case. In case of equals actually working though there are cases where compressed and uncompressed bytes differ for the same content which made for a slow and potentially very allocation heavy comparison. This commit (at the cost of some complexity) makes the equality checks needed to deduplicate Beats style metadata about twice as fast in isolation and more importantly saves a massive amount of allocations in them which should make for a larger practical speedup. This has not been a huge deal in practice yet, but I would like to use the functionality to implement metadata deduplication in a follow-up that is fairly simple but requires that the equals check in these objects is safe to run in a hot loop on the master thread. Relates #77466

elasticmachine · 2021-10-21T07:14:33Z

Pinging @elastic/es-distributed (Team:Distributed)

elasticmachine · 2021-10-21T07:14:33Z

Pinging @elastic/es-search (Team:Search)

server/src/main/java/org/elasticsearch/common/compress/CompressedXContent.java

…x-content-comparison

DaveCTurner

Looks good, I left a few small comments.

server/src/main/java/org/elasticsearch/common/compress/CompressedXContent.java

DaveCTurner · 2021-10-21T10:52:53Z

server/src/test/java/org/elasticsearch/common/compress/DeflateCompressedXContentTests.java

+        final CompressedXContent sameAsOne =
+            new CompressedXContent((builder, params) ->
+                builder.stringListField("arr", Arrays.asList(randomJSON)), XContentType.JSON, ToXContent.EMPTY_PARAMS);
+        assertEquals(one, sameAsOne);


Can we assert that the compressed bytes are not equal in this case?

Also we are not exercising equalsWhenUncompressed very hard in any of these tests since usually the bytes will be the same or the CRC is not going to match. Could we either construct some CRC collisions or else just test the method directly, checking in particular that does sometimes return false?

Ah I see Alan already approved this. I think this is a blocker, the rest of my comments are less important.

Added a test for this directly now :) I didn't want to hard code a collision (or brute force one to begin with :D) and finding one at runtime takes too long as well.

I'd quite like to have an assertFalse(Arrays.equals(one.compressed(), sameAsOne.compressed())); here too, mostly to document that the compressed representations are different.

server/src/main/java/org/elasticsearch/common/compress/CompressedXContent.java

DaveCTurner · 2021-10-21T11:25:14Z

server/src/test/java/org/elasticsearch/common/compress/DeflateCompressedXContentTests.java

+        final CompressedXContent sameAsOne =
+            new CompressedXContent((builder, params) ->
+                builder.stringListField("arr", Arrays.asList(randomJSON)), XContentType.JSON, ToXContent.EMPTY_PARAMS);
+        assertEquals(one, sameAsOne);


Ah I see Alan already approved this. I think this is a blocker, the rest of my comments are less important.

…x-content-comparison

original-brownbear · 2021-10-21T18:43:36Z

Thanks David, all points addressed now :)

…x-content-comparison

DaveCTurner

LGTM with two further suggestions.

DaveCTurner · 2021-10-21T19:37:36Z

server/src/test/java/org/elasticsearch/common/compress/DeflateCompressedXContentTests.java

+        final CompressedXContent sameAsOne =
+            new CompressedXContent((builder, params) ->
+                builder.stringListField("arr", Arrays.asList(randomJSON)), XContentType.JSON, ToXContent.EMPTY_PARAMS);
+        assertEquals(one, sameAsOne);


I'd quite like to have an assertFalse(Arrays.equals(one.compressed(), sameAsOne.compressed())); here too, mostly to document that the compressed representations are different.

DaveCTurner · 2021-10-21T19:38:56Z

server/src/test/java/org/elasticsearch/common/compress/DeflateCompressedXContentTests.java

+        final CompressedXContent two = new CompressedXContent((builder, params) ->
+                builder.stringListField("arr", Arrays.asList(randomJSON2)), XContentType.JSON, ToXContent.EMPTY_PARAMS);
+        assertFalse(CompressedXContent.equalsWhenUncompressed(one.compressed(), two.compressed()));
+    }


Here you go ~~(cheating slightly since the inputs aren't XContent, but I won't tell...)~~ that irked me, I found a JSON collision instead.

Suggested change

}

}

public void testEqualsCrcCollision() throws IOException {

final CompressedXContent content1 = new CompressedXContent("{\"d\":\"68&A<\"}".getBytes(StandardCharsets.UTF_8));

final CompressedXContent content2 = new CompressedXContent("{\"d\":\"gZG- \"}".getBytes(StandardCharsets.UTF_8));

assertEquals(content1.hashCode(), content2.hashCode()); // the inputs are a known CRC32 collision

assertNotEquals(content1, content2);

}

server/src/main/java/org/elasticsearch/common/compress/CompressedXContent.java

original-brownbear · 2021-10-22T08:18:29Z

Thanks David & Alan!

The hash code on this one is stable (we compute it from the uncompressed bytes). It's essentially free to compare it and early-break out in the false case. In case of equals actually working though there are cases where compressed and uncompressed bytes differ for the same content which made for a slow and potentially very allocation heavy comparison. This commit (at the cost of some complexity) makes the equality checks needed to deduplicate Beats style metadata about twice as fast in isolation and more importantly saves a massive amount of allocations in them which should make for a larger practical speedup. This has not been a huge deal in practice yet, but I would like to use the functionality to implement metadata deduplication in a follow-up that is fairly simple but requires that the equals check in these objects is safe to run in a hot loop on the master thread. Relates #77466

The hash code on this one is stable (we compute it from the uncompressed bytes). It's essentially free to compare it and early-break out in the false case. In case of equals actually working though there are cases where compressed and uncompressed bytes differ for the same content which made for a slow and potentially very allocation heavy comparison. This commit (at the cost of some complexity) makes the equality checks needed to deduplicate Beats style metadata about twice as fast in isolation and more importantly saves a massive amount of allocations in them which should make for a larger practical speedup. This has not been a huge deal in practice yet, but I would like to use the functionality to implement metadata deduplication in a follow-up that is fairly simple but requires that the equals check in these objects is safe to run in a hot loop on the master thread. Relates elastic#77466

elasticmachine added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Search Meta label for search team labels Oct 21, 2021

nicer

0b60dbe

romseygeek reviewed Oct 21, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/common/compress/CompressedXContent.java Show resolved Hide resolved

original-brownbear requested a review from romseygeek October 21, 2021 09:46

romseygeek approved these changes Oct 21, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/common/compress/CompressedXContent.java Show resolved Hide resolved

original-brownbear added 2 commits October 21, 2021 12:19

Merge remote-tracking branch 'elastic/master' into faster-compressed-…

625d519

…x-content-comparison

add comment

8e4809b

DaveCTurner reviewed Oct 21, 2021

View reviewed changes

DaveCTurner requested changes Oct 21, 2021

View reviewed changes

original-brownbear added 3 commits October 21, 2021 17:22

Merge remote-tracking branch 'elastic/master' into faster-compressed-…

9ab5636

…x-content-comparison

cleanup + additional test

2ebb636

renaming

e535427

original-brownbear requested a review from DaveCTurner October 21, 2021 18:43

Merge remote-tracking branch 'elastic/master' into faster-compressed-…

4b4ea40

…x-content-comparison

DaveCTurner approved these changes Oct 21, 2021

View reviewed changes

DaveCTurner reviewed Oct 21, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/common/compress/CompressedXContent.java Outdated Show resolved Hide resolved

add David's test

edf8bf2

original-brownbear merged commit 528bcb9 into elastic:master Oct 22, 2021

original-brownbear deleted the faster-compressed-x-content-comparison branch October 22, 2021 08:18

original-brownbear added backport pending and removed backport pending labels Oct 22, 2021

original-brownbear mentioned this pull request Oct 22, 2021

Speed up CompressedXContent.equals (#79600) #79672

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

danhermann added v7.16.0 and removed v7.16.1 labels Oct 27, 2021

original-brownbear restored the faster-compressed-x-content-comparison branch April 18, 2023 20:37

-    }
+    }
+    public void testEqualsCrcCollision() throws IOException {
+        final CompressedXContent content1 = new CompressedXContent("{\"d\":\"68&A<\"}".getBytes(StandardCharsets.UTF_8));
+        final CompressedXContent content2 = new CompressedXContent("{\"d\":\"gZG- \"}".getBytes(StandardCharsets.UTF_8));
+        assertEquals(content1.hashCode(), content2.hashCode()); // the inputs are a known CRC32 collision
+        assertNotEquals(content1, content2);
+    }

Speed up CompressedXContent.equals #79600

Speed up CompressedXContent.equals #79600

Uh oh!

Conversation

original-brownbear commented Oct 21, 2021

Uh oh!

elasticmachine commented Oct 21, 2021

Uh oh!

elasticmachine commented Oct 21, 2021

Uh oh!

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

original-brownbear Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DaveCTurner Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Oct 21, 2021

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

original-brownbear commented Oct 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DaveCTurner Oct 21, 2021 •

edited

Loading