Skip to content

Conversation

@tclavier
Copy link
Contributor

No description provided.

@jjasoncool
Copy link

jjasoncool commented Nov 29, 2023

I think maybe using "sed" to change original files?
https://github.com/nextcloud/docker/blob/master/27/fpm/Dockerfile#L99-L103

@tclavier
Copy link
Contributor Author

using sed in startup script to replace opcache.memory_consumption=128 by opcache.memory_consumption=${PHP_OPCACHE_MEMORY_CONSUMTION} ?

@jjasoncool
Copy link

using sed in startup script to replace opcache.memory_consumption=128 by opcache.memory_consumption=${PHP_OPCACHE_MEMORY_CONSUMTION} ?

Sorry, That all version was build by template files, your code is right.
But I think that can also add ENV jit_buffer_size also.

@J0WI
Copy link
Contributor

J0WI commented Jan 9, 2024

This requires some documentation in the README.

echo 'opcache.interned_strings_buffer=32'; \
echo 'opcache.max_accelerated_files=10000'; \
echo 'opcache.memory_consumption=128'; \
echo 'opcache.memory_consumption=${PHP_OPCACHE_MEMORY_CONSUMTION}'; \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo 'opcache.memory_consumption=${PHP_OPCACHE_MEMORY_CONSUMTION}'; \
echo 'opcache.memory_consumption=${PHP_OPCACHE_MEMORY_CONSUMPTION}'; \

echo 'opcache.interned_strings_buffer=32'; \
echo 'opcache.max_accelerated_files=10000'; \
echo 'opcache.memory_consumption=128'; \
echo 'opcache.memory_consumption=${PHP_OPCACHE_MEMORY_CONSUMTION}'; \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo 'opcache.memory_consumption=${PHP_OPCACHE_MEMORY_CONSUMTION}'; \
echo 'opcache.memory_consumption=${PHP_OPCACHE_MEMORY_CONSUMPTION}'; \

Copy link
Member

@joshtrichards joshtrichards left a comment

Choose a reason for hiding this comment

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

If we're going to support opcache changes like this, we should probably also support opcache.max_accelerated_files and opcache.interned_strings_buffer since they're all part of the same setup checks in Server. Otherwise it'll be an incomplete enhancement.

@joshtrichards joshtrichards changed the title use var PHP_OPCACHE_MEMORY_CONSUMTION for configuration use var PHP_OPCACHE_MEMORY_CONSUMPTION for configuration Feb 8, 2024
@J0WI
Copy link
Contributor

J0WI commented Apr 2, 2024

See also #2185 (comment)

# see https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html
ENV PHP_MEMORY_LIMIT 512M
ENV PHP_UPLOAD_LIMIT 512M
ENV PHP_OPCACHE_MEMORY_CONSUMTION 128
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ENV PHP_OPCACHE_MEMORY_CONSUMTION 128
ENV PHP_OPCACHE_MEMORY_CONSUMPTION 128

# see https://docs.nextcloud.com/server/latest/admin_manual/installation/server_tuning.html#enable-php-opcache
ENV PHP_MEMORY_LIMIT 512M
ENV PHP_UPLOAD_LIMIT 512M
ENV PHP_OPCACHE_MEMORY_CONSUMTION 128
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ENV PHP_OPCACHE_MEMORY_CONSUMTION 128
ENV PHP_OPCACHE_MEMORY_CONSUMPTION 128

@jessebot
Copy link
Contributor

This is a common request downstream in nextcloud/helm. If this PR was re-submitted with the suggested fixes here, would it still be viable? I see there was conversation #2185, but I can't tell if that means this PR should be closed?

@J0WI
Copy link
Contributor

J0WI commented Sep 18, 2024

Is it only about the memory_consumption setting?

@J0WI
Copy link
Contributor

J0WI commented Oct 8, 2024

see also #2275

@J0WI
Copy link
Contributor

J0WI commented Jan 9, 2025

@tclavier are you still working on this?

@tclavier
Copy link
Contributor Author

I guess I didn't understand what was missing for this PR to be accepted.

@J0WI
Copy link
Contributor

J0WI commented Jan 11, 2025

It requires documentation in README

@tclavier
Copy link
Contributor Author

I thinks all is OK, who can merge this PR ?

@J0WI J0WI merged commit e0294b6 into nextcloud:master Jan 16, 2025
18 of 21 checks passed
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.

5 participants