Skip to content

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Mar 22, 2022

FieldMemoryStats is a simple wrapper around the association of a field
to how much memory it is taking up. The size of this is not that large
(not millions of keys), and there is lots of other overhead, so hppc
classes are not really needed. This commit converts FieldMemoryStats to
use a HashMap internally.

relates #84735

FieldMemoryStats is a simple wrapper around the association of a field
to how much memory it is taking up. The size of this is not that large
(not millions of keys), and there is lots of other overhead, so hppc
classes are not really needed. This commit converts FieldMemoryStats to
use a HashMap internally.

relates elastic#84735
@rjernst rjernst added :Core/Infra/Core Core issues without another label >refactoring labels Mar 22, 2022
@rjernst rjernst requested a review from DaveCTurner March 22, 2022 17:45
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Mar 22, 2022
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Change looks correct to me but I think my only involvement in this area was to lift-and-shift the existing completion stats computation into a cache. I don't have a frame of reference to confirm that these maps are small enough to make this change without further investigation.

@rjernst
Copy link
Member Author

rjernst commented Mar 22, 2022

Perhaps @jpountz could take a look? This is used for tracking in fielddata and completion suggester.

@jpountz
Copy link
Contributor

jpountz commented Mar 23, 2022

Completion stats should be good, there would usually be one or two completion fields per mapping, very unlikely more.

Field data stats are more interesting. This is bounded by the number of keyword fields that users queried in a way that would require loading global ordinals. I'm confident that it would be ok with the new indexing strategy which bounds fields in mappings to fields that are actually useful. Filebeat/metricbeat indices can have fields in the order of several thousands, but it looks like we have logic to not put entries into the cache when the field does not exist in the Lucene index. So in both case, I'd expect these maps to have a number of entries in the order of a couple hundreds at most.

@rjernst rjernst merged commit c61f759 into elastic:master Mar 23, 2022
@rjernst rjernst deleted the hppc/fieldmemorystats branch March 23, 2022 13:32
@rjernst rjernst mentioned this pull request Mar 24, 2022
43 tasks
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 >refactoring Team:Core/Infra Meta label for core/infra team v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants