Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ingest field configuration helper cache #2614

Draft
wants to merge 7 commits into
base: integration
Choose a base branch
from

Conversation

dtspence
Copy link
Collaborator

Implementation of a field helper cache using an LRU map.

  • The cache can be optionally enabled on the BaseIngestHelper instance and will wrap an existing FieldConfigHelper implementation.
  • By default the cache is disabled and must be enabled via configuration.

Closes #2612

@dtspence dtspence force-pushed the feature/ingest-cache-field-helper branch from c35be9b to afdf5af Compare October 16, 2024 17:49
@dtspence dtspence force-pushed the feature/ingest-cache-field-helper branch 2 times, most recently from db63c0c to 3f7d5b6 Compare October 17, 2024 14:10
@dtspence dtspence marked this pull request as ready for review October 17, 2024 14:10

private static class ResultEntry {
private final String fieldName;
private final EnumMap<AttributeType,Boolean> resultMap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we not need to limit the size of this cache?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the map here will be limited to the size of the AttributeType (which was in the current class - maybe we should change the name). I tried an alternate version which used a switch statement on the enum and individual boolean variables, but it did not seem to impact the timing w/JMH. I will confirm the timing again - and we can also update to have a switch statement and explicit boolean results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An approach with a switch statement and primitive boolean variables was slightly quicker on second pass. I updated the logic, please let me know if we should evaluate the previous approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed additional performance ideas, going to run additional tests.

public void testCachingBehaviorWillCallBaseMethods() {
// @formatter:off
Stream.of(new Object[][] {
new Object[] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we actually calling all these pairs?

You are iterating over an array of arrays but there's only 1 inner array with 12 elements in it.
It seems like we want 6 inner arrays, each with two elements {action, verify}
I think we're actually only testing the isIndexOnlyField pair here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also has me wondering if maybe it's just easier to do the manual calls in the test instead of dealing with all the method references and such.

cachedHelper.isIndexOnlyField(fieldName);
verify(mockHelper).isIndexOnlyField(fieldName);

cachedHelper.isIndexedField(fieldName);
verify(mockHelper).isIndexedField(fieldName);

// ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is a bug, thank you for catching that. In regards to the manual tests - I updated, thank you.

@dtspence dtspence force-pushed the feature/ingest-cache-field-helper branch from 3f7d5b6 to 47d1cf3 Compare October 25, 2024 18:45
@dtspence dtspence marked this pull request as draft October 25, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FieldConfigHelper results cache
3 participants