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

fix: remove cpu core detection for preview semaphore #38385

Closed

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented May 21, 2023

Summary

The idea of automatically limiting the number of concurrent preview requests to the number of CPU cores is nice. Unfortunately it's difficult to determinate the number of CPU cores (e.g. open_basedir, freebsd, etc.).

This patch removes the detection and set the default for requests (existing + new) to 8 and generation to 4.

TODO

  • CI

Checklist

@kesselb kesselb force-pushed the fix-remove-auto-guessing-for-preview-semaphore branch from f15d336 to cb3861c Compare May 21, 2023 21:20
@kesselb kesselb self-assigned this May 21, 2023
@kesselb kesselb added bug 3. to review Waiting for reviews labels May 21, 2023
@kesselb
Copy link
Contributor Author

kesselb commented May 21, 2023

/backport to stable26

@kesselb kesselb requested review from szaimen, a team, ArtificialOwl, icewind1991 and nfebe and removed request for a team May 21, 2023 21:21
Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

This looks good but what if we maintain the checks on systems that we know how to?

I mean what if we add code that checks if the platform is one where we know how to detect the number of cores we just go ahead and do it?

I mean the feature would be beneficial for those on platforms which we support this feature on.

We can use something like PHP_OS_FAMILY see https://www.php.net/manual/en/reserved.constants.php

    switch(PHP_OS_FAMILY) {
       case 'Window':
            //check ?
       case 'BSD':
            //check?
        // More cases?
        
        default:
             //return the default preview counts without checking?
    }

@szaimen
Copy link
Contributor

szaimen commented May 22, 2023

/backport to stable27

@joshtrichards
Copy link
Member

joshtrichards commented Jun 6, 2023

This would certainly address the issue for those impacted by it.

But: what if we continue to try to determine as-is, but just make sure that a automated determination isn't a hard failure? (using the defaults when that happens). The other OSes/situations that are not currently functioning would then effectively function as this PR suggests.

I realize this feature is configurable, but the automatic determination is pretty nice considering the wide array of hardware platforms NC is used on. Maintaining the automation (which works for a sizable % of the user base) goes a long way towards a positive experience / first impression in a a fairly visible area of the platform.

@fenn-cs Unfortunately the checks would have to be more extensive than that because it's not just OS but also various local security restrictions.

@kesselb
Copy link
Contributor Author

kesselb commented Jun 6, 2023

If anyone wishes to take over, please go ahead ;)

The reason I'm removing it instead of extending it is because a configuration option is all you need to get the function going. Everything else, like finding out how many CPU cores there are, is optional. Please keep in mind that this optional part, to find out how many CPU cores are there, is executed every time a generator instance is requested. You can't even turn it off. I suggest setting up a setup check to let the admin know. If it is possible, we could even suggest a value.

@kesselb kesselb force-pushed the fix-remove-auto-guessing-for-preview-semaphore branch from cb3861c to 772f932 Compare June 6, 2023 13:54
@kesselb kesselb added this to the Nextcloud 28 milestone Jun 13, 2023
@kesselb kesselb force-pushed the fix-remove-auto-guessing-for-preview-semaphore branch from 772f932 to 6c78f78 Compare June 13, 2023 17:09
@kesselb kesselb force-pushed the fix-remove-auto-guessing-for-preview-semaphore branch from 6c78f78 to 939ef10 Compare June 22, 2023 19:37
The idea of automatically limiting the number of concurrent preview requests to the number of CPU cores is nice.
Unfortunately it's difficult to determinate the number of CPU cores (e.g. open_basedir, freebsd, etc.).

This patch removes the detection and set the default for requests (existing + new) to 8 and generation to 4.

Signed-off-by: Daniel Kesselberg <[email protected]>
@kesselb kesselb force-pushed the fix-remove-auto-guessing-for-preview-semaphore branch from 939ef10 to 30f8c07 Compare July 21, 2023 15:21
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@Altahrim Altahrim mentioned this pull request Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 14, 2024
This was referenced Mar 18, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@kesselb kesselb added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 28, 2024
@kesselb kesselb modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: flood of errors is_file(): open_basedir restriction in effect. File(/proc/cpuinfo) after 25 > 26 update
7 participants