Skip to content

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Feb 28, 2025

We can avoid using the volatile field when there is only a single default project.

Relates: #123662

We can avoid using the volatile field when there is only a single
default project.

Relates: elastic#123662
@ywangd ywangd added >non-issue :Core/Infra/Core Core issues without another label v9.1.0 labels Feb 28, 2025
@ywangd ywangd requested review from tvernum and a team February 28, 2025 02:51
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Feb 28, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Comment on lines +1851 to +1853
if (isSingleProject()) {
final var project = projectMetadata.get(DEFAULT_PROJECT_ID);
return project.hasIndex(index) ? Optional.of(project) : Optional.empty();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the optimization can be applied to any single project, not only just the default. But I didn't do it for two reasons:

  1. Existing code only handles single-default project
  2. No great way to get the single project if it is not the default. We can use projectMetadata.values().iterator().next(). But that feels quite a bit overhead since we are talking about a few cpu cycles here.

@tvernum
Copy link
Contributor

tvernum commented Feb 28, 2025

Are you sure this is cheaper?
If we do multiple lookups (which is the most common case) then it makes multiple calls to isSingleProject() and retrieves the ProjectMetadata multiple times.
Is that really cheaper than a volatile read?

@ywangd
Copy link
Member Author

ywangd commented Feb 28, 2025

cheaper than a volatile read

If we do multiple lookups, shouldn't the existing code do multiple volatile read?

I cannot be certain about the performance without benchmarking. That said, I think there is a good chance for this change to be cheaper for single-default project setup since both size and containsKey checks should be fairly cheap for a single-element map. For multi-project setup, it performs an extra size check compared to the existing code which is, strictly speadking, more expensive.

In any case, there is no need to merge this before benchmarking is avilable for changes in #123662.

@ywangd
Copy link
Member Author

ywangd commented Feb 28, 2025

Let me change this to draft so that it does not get merged accidentally.

@ywangd ywangd marked this pull request as draft February 28, 2025 04:46
@ywangd
Copy link
Member Author

ywangd commented Feb 28, 2025

Also, the existing code can write to the volatile field multiple times. It is an edge case but not impossible when the concurrency is high.

@nielsbauman
Copy link
Contributor

What if we compute the value of isSingleProject() in the constructor and store it as a field? That's a low memory overhead but saves some lookups.

We could take it even further by, instead of storing it as a boolean, storing the default/single field as @Nullable private final ProjectMetadata singleProject;. That would avoid even more lookups. But that one's probably a bit overkill.

@ywangd
Copy link
Member Author

ywangd commented Mar 3, 2025

What if we compute the value of isSingleProject()

I actually started with this but was not sure whether it would deem to be too hacky. Personally I don't mind it. It would make the single project scenario more generic to cover not only a single "default" project but also any single project, which could be useful in future when a single big project needs to take a cluster for its own.

@nielsbauman
Copy link
Contributor

I wouldn't consider that to be hacky. I see it as just pre-computing/caching a value, which we do in tons of places.

@ywangd
Copy link
Member Author

ywangd commented Mar 3, 2025

I thought about hacky because it would be the only instance filed that is not de/serialized for an essentially "record" object. I remember having discussion in the past about this being no good. But as commented earlier, I don't personally mind it.

@nielsbauman
Copy link
Contributor

Both indicesLookup and projectLookup aren't (de)serialized, so I think we have precedent to do this.

@ywangd
Copy link
Member Author

ywangd commented Mar 3, 2025

Thanks you are right. I forgot most of Metadata is now with ProjectMetadata. With ProjectMetadata, the oldestIndexVersionId field is a great example since it is private final same to what would be used for the field here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v9.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants