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

[BUG] FeatureFlags.isEnabled is kinda slow #16519

Open
andrross opened this issue Oct 29, 2024 · 7 comments · May be fixed by #17611
Open

[BUG] FeatureFlags.isEnabled is kinda slow #16519

andrross opened this issue Oct 29, 2024 · 7 comments · May be fixed by #17611
Assignees
Labels
bug Something isn't working good first issue Good for newcomers Search Search query, autocomplete ...etc v3.0.0 Issues and PRs related to version 3.0.0

Comments

@andrross
Copy link
Member

andrross commented Oct 29, 2024

Describe the bug

After having run the full big5 benchmark to create the index, I ran the following command to isolate a simple query and run it as fast as possible:

opensearch-benchmark execute-test \
  --target-hosts localhost:9200 \
  --workload big5 --workload-params 'number_of_replicas:0,target_throughput:0,search_clients:8,test_iterations:100000' \
  --pipeline benchmark-only --include-tasks term \
  --kill-running-processes

This is a super fast query, so I was specifically looking for overhead in things not related to doing heavyweight search work. It turns out FeatureFlags.isEnabled is taking almost 5% of CPU cycles. It is unsurprising that a feature flag check might pop up in hot path code, and there's no reason that looking up a setting value should be so expensive. It turns out the System.getProperty check is kind of slow (because of a security manager check it seems).

Related component

Search

To Reproduce

See above

Expected behavior

Features flags defined in system properties are not dynamic. All feature flags are statically known. We should pre-populate these booleans into an immutable map during static initialization to avoid the System.getProperty call at runtime.

Additional Details

image
@andrross andrross added bug Something isn't working untriaged labels Oct 29, 2024
@github-actions github-actions bot added the Search Search query, autocomplete ...etc label Oct 29, 2024
@andrross andrross added the good first issue Good for newcomers label Oct 29, 2024
@msfroh
Copy link
Collaborator

msfroh commented Oct 29, 2024

A comment was added as part of #4959 suggesting that we remove that if statement once feature flags can only be set via opensearch.yml.

I think we may need to fix FeatureFlagSetter, which uses system properties to dynamically update feature flags in unit tests.

@andrross
Copy link
Member Author

I think FeatureFlagSetter can probably be removed and replaced with calls to FeatureFlags.initializeFeatureFlags, which seems to be more widely used in tests.

@msfroh
Copy link
Collaborator

msfroh commented Oct 30, 2024

Yeah, that makes sense. We can clear the flags in OpenSearchTestCase#tearDown() using FeatureFlags.initializeFeatureFlags. The automatic clearing of feature flags on teardown was arguably the most useful part of FeatureFlagSetter.

@bhngupta
Copy link

bhngupta commented Nov 8, 2024

This seems interesting, can I take this up?

@dbwiddis
Copy link
Member

dbwiddis commented Nov 8, 2024

This seems interesting, can I take this up?

In the Open Source world the answer is almost always, universally, an all-caps-shouted YES!

Assigning to you!

@sandeshkr419
Copy link
Contributor

[Search Triage] @bhngupta Hi, were you able to get started on this?

@getsaurabh02 getsaurabh02 added the v3.0.0 Issues and PRs related to version 3.0.0 label Feb 26, 2025
@finnegancarroll
Copy link
Contributor

Taking a look at this.

@finnegancarroll finnegancarroll moved this from Todo to In-Review in Performance Roadmap Mar 17, 2025
@finnegancarroll finnegancarroll linked a pull request Mar 17, 2025 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers Search Search query, autocomplete ...etc v3.0.0 Issues and PRs related to version 3.0.0
Projects
Status: In-Review
Status: 🆕 New
Development

Successfully merging a pull request may close this issue.

7 participants