Prevent multiple sets copies while adding index aliases

[idegtiarenko]

Prior to this change we were copying aliases map for every index change in the builder at:

elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java

Lines 1925 to 1929 in 29c5b49

	Set<Index> indices = new HashSet<>(aliasedIndices.getOrDefault(alias, Set.of()));
	if (indices.add(index) == false) {
	return this; // indices already contained this index
	}
	aliasedIndices.put(alias, Collections.unmodifiableSet(indices));

This is becoming very expensive when applying a MetadataDiff as part of this operation is adding all indices one by one to the builder. In particular this results in N copies for the underlying set when an alias references N indices (this situation is fairly common with data streams).

Closes: #110217

[elasticsearchmachine]

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

[ywangd]

I have a comment mostly for my education.

[ywangd]

I am not familiar with ImmutableOpenMap and ObjectObjectHashMap. So I am not sure whether we'd prefer these new methods especially the one that access and assign directly to mutableMap.values. I think it is likely more efficient since it avoids allocate another map. But to play on the safe side, I'd just use a regular map in Metadata#Builder#build and then create an ImmutableOpenMap from it with something like the follows:

// use a HashMap `m` to populate aliases

var mb = ImmutableOpenMap.<String, Set<Index>>builder(m.size());
m.forEach((k, v) -> mb.put(k, Collections.unmodifiableSet(v)));
...

It has one more map allocation. But that feels acceptable and we don't need to touch anything in ImmutableOpenMap.

[idegtiarenko]

The main reason for using ImmutableOpenMap and extending it with new operations was to avoid copying map when building. Example above effectively copies map in .forEach call in the end. In contrast ImmutableOpenMap builder reuses internal structure:

elasticsearch/server/src/main/java/org/elasticsearch/common/collect/ImmutableOpenMap.java

Lines 350 to 352 in a59c182

	ObjectObjectHashMap<KType, VType> mutableMap = this.mutableMap;
	this.mutableMap = null; // null out the map so that you can't reuse this builder
	return mutableMap.isEmpty() ? of() : new ImmutableOpenMap<>(mutableMap);

Overall I do not have a strong preference for either of approaches. Both of them would result in less copying than we have today.

[ywangd]

Yeah your change is definitely more efficient. It does not copy but still has re-assignment after the transform. So it may not be far off. I was not sure what was the best pratice with hppc maps. I vaguely remembered it being necessary for performance. So let's not use a regular HashMap.

I google'd a bit and it seems the official site actually promotes direct buffer access as the fastest approach. But it also checks an allocated field since not every element in values is assigned. But the website is outdated and there is no long such field. Their GitHub examples no longer has such usages but instead uses the iterator which has the following code at its heart:

    protected ObjectObjectCursor<KType, VType> fetch() {
      if (slot < max) {
        KType existing;
        for (slot++; slot < max; slot++) {
          if (!((existing = (KType) keys[slot]) == null)) {
            cursor.index = slot;
            cursor.key = existing;
            cursor.value = (VType) values[slot];
            return cursor;
          }
        }
      }

      if (slot == max && hasEmptyKey) {
        cursor.index = slot;
        cursor.key = null;
        cursor.value = (VType) values[max];
        slot++;
        return cursor;
      }

      return done();
    }

I am not entirely sure whether we should use it since its performance is likely worse than directly manipulating the arrays. But it feels safer. Or alternatively we should add the null check like how the iterator does it in your version.

[nicktindall]

LGTM (pending addressing @ywangd's concerns, which I don't have a strong opinion about)

[original-brownbear]

LGTM from my end, I think this is close to as fast as it can get and a nice simplification!

[ywangd]

LGTM
Thanks for the iteration!

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	8.16
✅	8.15
✅	8.x