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

THREESCALE-7941 - Review warning displayed in apicast logs about variables_hash_max_size & variables_hash_bucket_size #1395

Merged
merged 2 commits into from
May 2, 2023

Conversation

ernaniaz
Copy link
Contributor

@ernaniaz ernaniaz commented Mar 7, 2023

Increasing the limit of variables_hash_bucket_size make the warning message disapear.

This message is a warning that NGiNX display when the used variables
didn't fit the buffer. Increasing will also increase performance and
will not show this warning.

The NGiNX documenation about hashes says:

To quickly process static sets of data such as server names, map
directive’s values, MIME types, names of request header strings, nginx
uses hash tables. During the start and each re-configuration nginx
selects the minimum possible sizes of hash tables such that the bucket
size that stores keys with identical hash values does not exceed the
configured parameter (hash bucket size). The size of a table is
expressed in buckets. The adjustment is continued until the table size
exceeds the hash max size parameter. Most hashes have the corresponding
directives that allow changing these parameters, for example, for the
server names hash they are server_names_hash_max_size and
server_names_hash_bucket_size.

The hash bucket size parameter is aligned to the size that is a multiple
of the processor’s cache line size. This speeds up key search in a hash
on modern processors by reducing the number of memory accesses. If hash
bucket size is equal to one processor’s cache line size then the number
of memory accesses during the key search will be two in the worst case —
first to compute the bucket address, and second during the key search
inside the bucket. Therefore, if nginx emits the message requesting to
increase either hash max size or hash bucket size then the first
parameter should first be increased.

Reference: NGiNX: Setting up hashes

Also, to check the processor's cache line size, use:

cat /sys/devices/system/cpu/cpu*/cache/index*/coherency_line_size | head -n 1

Source: https://stackoverflow.com/questions/794632/programmatically-get-the-cache-line-size

This commit fix the Jira issue THREESCALE-7941

@ernaniaz ernaniaz requested a review from a team as a code owner March 7, 2023 20:34
Copy link
Member

@kevprice83 kevprice83 left a comment

Choose a reason for hiding this comment

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

Try solving the issue with one change, if not possible then add the second change. Values printed in the logs are current values not suggested new values.

@@ -129,7 +129,7 @@ local function resty(code)

if not cmd then return nil, 'could not find resty' end

local handle = io.popen(([[/usr/bin/env -i %q -e %q]]):format(cmd, code))
local handle = io.popen(([[/usr/bin/env -i %q -e %q 2>/dev/null]]):format(cmd, code))
Copy link
Member

Choose a reason for hiding this comment

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

This code change looks unrelated to this issue. I suspect it's unintended to be included in this commit and will cause a conflict with other PRs or with this one depending which gets merged first.

@@ -43,7 +43,8 @@ http {
tcp_nodelay on;
server_tokens off;


variables_hash_bucket_size 128;
variables_hash_max_size 1024;
Copy link
Member

Choose a reason for hiding this comment

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

There is no point in declaring variable_hash_max_size 1024; because that is already the default value. We can see that in the log output attached in the JIRA.

I think we need to just follow Nginx documentation here:

if nginx emits the message requesting to increase either hash max size or hash bucket size then the first parameter should first be increased.

variables_hash_max_size 2048;

If that fixes the issue we are good, if not then declare:

variables_hash_bucket_size 256;

At least this is how I understand what Nginx documentation is suggesting to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake with the PR, it got the THREESCALE-7942 together, just ignore the first commit.
I added variables_hash_max_size with the default value because both configurations are used together, keep it clear what's configured, and there's no problem to specify some variable with their default value.

Copy link
Member

Choose a reason for hiding this comment

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

According to the doc:

The hash bucket size parameter is aligned to the size that is a multiple of the processor’s cache line size. This speeds up key search in a hash on modern processors by reducing the number of memory accesses

So, as I understand, the variables_hash_bucket_size is computed internally to be aligned with the processor’s cache line size, it does not have a fixed default size. We should not set it.

@eguzki
Copy link
Member

eguzki commented Mar 9, 2023

I suggest that THREESCALE-7941 branch is based on master branch instead of THREESCALE-7942 branch. Not a big deal, but better to manage the changes.

I can help with the rebase stuff if you need.

@eguzki
Copy link
Member

eguzki commented Mar 9, 2023

Verification

  • Setup env
make development
make dependencies
  • Minimal apicast config
cat <<EOF >config.json
{
   "services": [
      {
         "proxy": {
             "hosts": ["one"],
             "proxy_rules": [],
             "api_backend": "https://echo-api.3scale.net",
             "policy_chain": []
         }
      }
   ]
}
EOF
  • Run apicast
bash-4.4$ APICAST_LOG_LEVEL=warn APICAST_WORKER=1 THREESCALE_CONFIG_FILE=config.json BACKEND_ENDPOINT_OVERRIDE=http://localhost:8081 ./bin/apicast
loading production environment configuration: /opt/app-root/src/gateway/config/production.lua

The warning message is gone.

@eguzki
Copy link
Member

eguzki commented Apr 24, 2023

@kevprice83 waiting for your approval (since you requested changes)

Copy link
Member

@kevprice83 kevprice83 left a comment

Choose a reason for hiding this comment

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

Yes this looks better now and aligned with what the Nginx documentation recommends. Nice work!

@kevprice83 kevprice83 merged commit fdf5b2e into master May 2, 2023
@kevprice83 kevprice83 deleted the THREESCALE-7941 branch May 2, 2023 14:16
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.

None yet

3 participants