search

opensearch-project / OpenSearch

🔎 Open source distributed and RESTful search engine.
https://opensearch.org/docs/latest/opensearch/index/
Apache License 2.0
9.85k stars 1.83k forks source link

[BUG] `FeatureFlags.isEnabled` is kinda slow #16519

Open andrross opened 3 weeks ago

andrross commented 3 weeks ago

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
msfroh commented 3 weeks ago

A comment was added as part of https://github.com/opensearch-project/OpenSearch/pull/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 commented 3 weeks ago

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

msfroh commented 3 weeks ago

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 commented 2 weeks ago

This seems interesting, can I take this up?

dbwiddis commented 2 weeks ago

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!