Limit frequency of feature last-used time updates

[joegallo]

I ran into this while benchmarking some search code that uses a licensed feature. There was enough lock contention on updating the last used time for that feature that it showed up in a lock flamegraph, so I added this logic to update the last used time less often. I'm not attached to ten seconds, I think we would probably see an improvement if we even just limit it to once a second. 🤷 (edit: I changed the logic.)

For most features, I don't think this would make a difference, but I know for example that the redact processor is running a check on every document, so I could see how the existing logic could cause a problem.

It's not useful to have multiple threads banging away on the map just to put the same value in repeatedly, so now the logic is that the last used time is only updated if it has increased (or if it isn't present at all, as it would be on a fresh start).

[elasticsearchmachine]

Pinging @elastic/es-security (Team:Security)

[elasticsearchmachine]

Hi @joegallo, I've created a changelog YAML for you.

[joegallo]

This test failure is real:

./gradlew ":x-pack:plugin:security:qa:multi-cluster:bcUpgradeTest" -Dtests.class="org.elasticsearch.xpack.remotecluster.RemoteClusterSecurityEsqlIT" -Dtests.bwc.main.version=9.2.0-SNAPSHOT -Dtests.bwc.refspec.main=62c84a486bd9710cc2f8a18441a30af77a121f5b

If we end up doing something like what this PR is proposing, then we'd need to address this test assertion:

elasticsearch/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityEsqlIT.java

Lines 434 to 436 in 1f9b6bb

	if (ccsLastUsedTimestampAtStartOfTest != null) {
	assertThat(lastUsed.toString(), not(equalTo(ccsLastUsedTimestampAtStartOfTest.toString())));
	}

[mark-vieira]

I think we would probably see an improvement if we even just limit it to once a second. 🤷

Perhaps it's worth doing the same benchmarks with different intervals to see where we get diminishing returns. I agree that 10 seconds is probably too coarse.

If we end up doing something like what this PR is proposing, then we'd need to address this test assertion:

That assertion doesn't seem to provide much value from what I can tell. I suspect we can relax it or remove it entirely.

[joegallo]

Heh, it's possible the value might even just be if the logic is "don't bash the same value in repeatedly!" (i.e. if the minimum millis diff is just 1). Anyway, I'll run my benchmark a few times and see what I find.

[joegallo]

Heh, it's possible the value might even just be if the logic is "don't bash the same value in repeatedly!" (i.e. if the minimum millis diff is just 1). Anyway, I'll run my benchmark a few times and see what I find.

Heh, yeah, at least for the feature I'm benchmarking, if we change it to only updating the usage timestamp if the timestamp has increased, then the lock contention falls out completely. See bfdeb0c.

[szybia]

can't find any fault with this approach

[elasticsearchmachine]

Hi @joegallo, I've updated the changelog YAML for you.

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	9.0
✅	9.1
✅	8.19

[joegallo]

It turns out that this was related to #111473.