Optimize the order of bytes in uuids for better compression. #24615

jpountz · 2017-05-11T11:50:14Z

Flake ids organize bytes in such a way that ids are ordered. However, we do not
need that property and could reorganize bytes in an order that would better suit
Lucene's terms dict instead.

Some synthetic tests suggest that this change decreases the disk footprint of
the _id field by about 50% in many cases (see UUIDTests.testCompression).
For instance, when simulating the indexing of 10M docs at a rate of 10k docs
per second, the current uid generator used 20.2 bytes per document on average,
while this new generator which only puts bytes in a different order uses 9.6
bytes per document on average.

We had already explored this idea in #18209 but the attempt to share long common
prefixes had had a bad impact on indexing speed. This time I have been more
careful about putting discriminant bytes early in the _id in a way that
preserves indexing speed on par with today, while still allowing for better
compression.

nik9000 · 2017-05-11T11:54:48Z

core/src/main/java/org/elasticsearch/action/index/IndexRequest.java

-            id(UUIDs.base64UUID());
+            String uid;
+            if (indexCreatedVersion == null) {
+                // we don't know the creation version yet, so default to random uuids, which are the only safe choice


Can you explain in the comment what "safe" means in this context? Like, it is the only choice that is guaranteed not to overlap or it is the only choice guaranteed to be fast-ish?

This is a part that I wanted to discuss with someone who better understands implicit index creation when the index of an index request does not exist yet. Is there a way to know what its created version will be?

The issue is that I could not change the uid generation algorithm in place as there would be a rare chance that uids would collide. So I'm looking for ways to make things work when the index is not created. Using a random uid should work, but I wonder whether we could do better.

I rephrased to mention collisions.

nik9000 · 2017-05-11T11:55:09Z

core/src/main/java/org/elasticsearch/common/LegacyTimeBasedUUIDGenerator.java

+
+/** These are essentially flake ids (http://boundary.com/blog/2012/01/12/flake-a-decentralized-k-ordered-unique-id-generator-in-erlang) but
+ *  we use 6 (not 8) bytes for timestamp, and use 3 (not 2) bytes for sequence number. */
+


Extra space

nik9000 · 2017-05-11T11:58:55Z

core/src/main/java/org/elasticsearch/common/TimeBasedUUIDGenerator.java

-        }
+    // protected for testing
+    protected long currentTimeMillis() {
+        return System.currentTimeMillis();


Is it worth looking at nanotime which doesn't go backwards?

That would not work as we need some absolute times. For instance you switch to a different JVM that uses a different origin for nanotime, you might reuse existing ids.

What about initializing to System.currentTimeMillis() on startup and using that time + System.nanoTime() on generation? Regardless, if it is worth doing at all it can wait for a followup.

yeah, maybe we should focus on the order of bytes here and rethink generation separately?

yeah, maybe we should focus on the order of bytes here and rethink generation separately?

++

nik9000 · 2017-05-11T12:03:55Z

core/src/main/java/org/elasticsearch/common/TimeBasedUUIDGenerator.java

+
+    // protected for testing
+    protected byte[] macAddress() {
+        return SECURE_MUNGED_ADDRESS;
    }

    @Override
    public String getBase64UUID()  {
        final int sequenceId = sequenceNumber.incrementAndGet() & 0xffffff;


Can you skip the masking given the way you are using the sequence id?

no, because of the if (sequenceId == 0) { check below

nik9000 · 2017-05-11T12:06:21Z

core/src/test/java/org/elasticsearch/common/UUIDTests.java

+    }
+
+    private static double testCompression(int numDocs, int numDocsPerSecond, int numNodes, Logger logger) throws Exception {
+        final double intervalBetweenDocs = 1000. / numDocsPerSecond; // milliseconds


1000.0 is easier for me to read.

nik9000 · 2017-05-11T12:08:58Z

core/src/main/java/org/elasticsearch/common/TimeBasedUUIDGenerator.java

-        putLong(uuidBytes, sequenceId, 12, 3);
-
-        assert 9 + SECURE_MUNGED_ADDRESS.length == uuidBytes.length;
+        // The order of bytes is important as Lucene uses a block tree (similar to a burst trie)


I love the explanations.

Indexing ids in binary form should help with indexing speed since we would have to compare fewer bytes upon sorting, should help with memory usage of the live version map since keys will be shorter, and might help with disk usage depending on how efficient the terms dictionary is at compressing terms. Since we can only expect base64 ids in the auto-generated case, this PR tries to use an encoding that makes the binary id equal to the base64-decoded id in the majority of cases (253 out of 256). It also specializes numeric ids, since this seems to be common when content that is stored in Elasticsearch comes from another database that uses eg. auto-increment ids. Another option could be to require base64 ids all the time. It would make things simpler but I'm not sure users would welcome this requirement. This PR should bring some benefits, but I expect it to be mostly useful when coupled with something like elastic#24615. Closes elastic#18154

Indexing ids in binary form should help with indexing speed since we would have to compare fewer bytes upon sorting, should help with memory usage of the live version map since keys will be shorter, and might help with disk usage depending on how efficient the terms dictionary is at compressing terms. Since we can only expect base64 ids in the auto-generated case, this PR tries to use an encoding that makes the binary id equal to the base64-decoded id in the majority of cases (253 out of 256). It also specializes numeric ids, since this seems to be common when content that is stored in Elasticsearch comes from another database that uses eg. auto-increment ids. Another option could be to require base64 ids all the time. It would make things simpler but I'm not sure users would welcome this requirement. This PR should bring some benefits, but I expect it to be mostly useful when coupled with something like #24615. Closes #18154

jpountz · 2017-07-07T15:55:17Z

I've been working on that PR again now that #25352 is merged. I updated it a bit so that it should improve indexing speed a bit more and help a bit less with compression (but still better than current master). @s1monw would you mind having a look?

jpountz · 2017-07-10T07:15:36Z

@elasticmachine please test it

s1monw

I left some comments, I also wonder in addition to what we do today should we maybe pass on the timestamp that is used to create the index. this is always consistent even across node restarts and can give us a common base? just an idea.

s1monw · 2017-07-10T08:06:30Z

core/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

@@ -290,12 +291,14 @@ protected void doRun() throws Exception {
                        case INDEX:
                            IndexRequest indexRequest = (IndexRequest) docWriteRequest;
                            MappingMetaData mappingMd = null;
+                            Version indexCreated = null;
                            final IndexMetaData indexMetaData = metaData.index(concreteIndex);
                            if (indexMetaData != null) {


IMO this can never happen. if you at what we do here we add a failure and skip this entire block when then index metadata is not available. I think this is legacy and can / must be removed. I still think @bleskes should confirm that since he knows this class best

+1. addFailureIfIndexIsUnavailable does these checks already.

thanks @bleskes

s1monw · 2017-07-10T08:06:57Z

core/src/main/java/org/elasticsearch/action/index/IndexRequest.java

@@ -484,7 +485,7 @@ public VersionType versionType() {
    }


-    public void process(@Nullable MappingMetaData mappingMd, String concreteIndex) {
+    public void process(@Nullable Version indexCreatedVersion, @Nullable MappingMetaData mappingMd, String concreteIndex) {


I wonder, given my comment about the null index metadata if this should not be nullable?

s1monw · 2017-07-10T08:07:54Z

core/src/main/java/org/elasticsearch/common/TimeBasedUUIDGenerator.java

-        }
+    // protected for testing
+    protected long currentTimeMillis() {
+        return System.currentTimeMillis();


yeah, maybe we should focus on the order of bytes here and rethink generation separately?

++

s1monw · 2017-07-10T08:25:15Z

core/src/main/java/org/elasticsearch/common/LegacyTimeBasedUUIDGenerator.java

+        final int sequenceId = sequenceNumber.incrementAndGet() & 0xffffff;
+        long timestamp = System.currentTimeMillis();
+
+        synchronized (this) {


I wonder why we don't make lastTimestamp an AtomicLong and use compareAndSet since we don't care if the compare and set was successful as long as we are moving forward? something like this:

long currentTimestamp = System.currentTimeMillis(); long lastUsedTimestamp = lastTimestamp.get(); long timestamp = Math.max(currentTimestamp, lastUsedTimestamp); if (sequenceId == 0) { timestamp++; } lastTimestamp.compareAndSet(timestamp, lastUsedTimestamp);

if that doesn't work and we really need to block or have consistent values aren't we vulnerable in terms of collisions on the same timestamp?

I'm working on this for a separate PR if you don't mind.

Thinking about it more, it would ensure that lastUsedTimestamp is greater than it was before, but not necessarily greater than timestamp. So there is a possibility of collision?

but don't we have the sequenc ID for this pupose. there are 2^31 docs to be indexed before you have a collision. I mean if System.currentTimeInMillis() goes backwards we reuse the same timestamp all the time? I am not sure how this would work at all if we can't sustain that?

if System.currentTimeInMillis() goes backwards we reuse the same timestamp all the time

no, since we force the clock to tick forward whenever the sequence id is 0?

there are 2^31 docs to be indexed before you have a collision

2^24 actually, we only use the 3 lower bytes of the sequence id

jpountz · 2017-07-10T10:31:04Z

I pushed more commits.

jpountz · 2017-07-10T14:06:24Z

@elasticmachine please test it

s1monw

LGTM we can fix the sync issue if we want in a followup it's and impl detail

Flake ids organize bytes in such a way that ids are ordered. However, we do not need that property and could reorganize bytes in an order that would better suit Lucene's terms dict instead. Some synthetic tests suggest that this change decreases the disk footprint of the `_id` field by about 50% in many cases (see `UUIDTests.testCompression`). For instance, when simulating the indexing of 10M docs at a rate of 10k docs per second, the current uid generator used 20.2 bytes per document on average, while this new generator which only puts bytes in a different order uses 9.6 bytes per document on average. We had already explored this idea in elastic#18209 but the attempt to share long common prefixes had had a bad impact on indexing speed. This time I have been more careful about putting discriminant bytes early in the `_id` in a way that preserves indexing speed on par with today, while still allowing for better compression.

This reverts commit 374cc7e898d03ac0db5a06dc4e750ae936014ae9.

* master: Add an underscore to flood stage setting Avoid failing install if system-sysctl is masked Add another parent value option to join documentation (elastic#25609) Ensure we rewrite common queries to `match_none` if possible (elastic#25650) Remove reference to field-stats docs. Optimize the order of bytes in uuids for better compression. (elastic#24615) Fix BytesReferenceStreamInput#skip with offset (elastic#25634) Limit the number of concurrent shard requests per search request (elastic#25632)

* master: (181 commits) Use a non default port range in MockTransportService Add a shard filter search phase to pre-filter shards based on query rewriting (elastic#25658) Prevent excessive disk consumption by log files Migrate RestHttpResponseHeadersIT to ESRestTestCase (elastic#25675) Use config directory to find jvm.options Fix inadvertent rename of systemd tests Adding basic search request documentation for high level client (elastic#25651) Disallow lang to be used with Stored Scripts (elastic#25610) Fix typo in ScriptDocValues deprecation warnings (elastic#25672) Changes DocValueFieldsFetchSubPhase to reuse doc values iterators for multiple hits (elastic#25644) Query range fields by doc values when they are expected to be more efficient than points. Remove SearchHit#internalHits (elastic#25653) [DOCS] Reorganized the highlighting topic so it's less confusing. Add an underscore to flood stage setting Avoid failing install if system-sysctl is masked Add another parent value option to join documentation (elastic#25609) Ensure we rewrite common queries to `match_none` if possible (elastic#25650) Remove reference to field-stats docs. Optimize the order of bytes in uuids for better compression. (elastic#24615) Fix BytesReferenceStreamInput#skip with offset (elastic#25634) ...

jpountz added :Internal >enhancement v6.0.0 labels May 11, 2017

nik9000 reviewed May 11, 2017

View reviewed changes

jpountz mentioned this pull request Jun 22, 2017

Index ids in binary form. #25352

Merged

jpountz force-pushed the enhancement/better_uuids branch from 584cc1e to 0d1b899 Compare July 7, 2017 15:52

jpountz requested a review from s1monw July 7, 2017 15:55

s1monw suggested changes Jul 10, 2017

View reviewed changes

jpountz force-pushed the enhancement/better_uuids branch from 4a7b675 to 9fd5ad5 Compare July 10, 2017 09:28

s1monw approved these changes Jul 11, 2017

View reviewed changes

jpountz added 8 commits July 11, 2017 17:26

iter

f87dfdf

iter

373253e

iter

d8c2ac6

iter

c9e14ab

Replace sync block with AtomicLong.compareAndSet.

723ca50

Revert "Replace sync block with AtomicLong.compareAndSet."

e1584ec

This reverts commit 374cc7e898d03ac0db5a06dc4e750ae936014ae9.

Fix version constant.

837d88e

jpountz force-pushed the enhancement/better_uuids branch from cce0caa to 837d88e Compare July 11, 2017 15:28

jpountz merged commit f9fbce8 into elastic:master Jul 11, 2017

jpountz deleted the enhancement/better_uuids branch July 11, 2017 15:28

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017


		/** These are essentially flake ids (http://boundary.com/blog/2012/01/12/flake-a-decentralized-k-ordered-unique-id-generator-in-erlang) but
		* we use 6 (not 8) bytes for timestamp, and use 3 (not 2) bytes for sequence number. */

Optimize the order of bytes in uuids for better compression. #24615

Optimize the order of bytes in uuids for better compression. #24615

Uh oh!

Conversation

jpountz commented May 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz commented Jul 7, 2017

Uh oh!

jpountz commented Jul 10, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz commented Jul 10, 2017

Uh oh!

jpountz commented Jul 10, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!