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

manual PATH scanner instead of command #614

Closed
wants to merge 7 commits into from

Conversation

divinity76
Copy link
Contributor

@divinity76 divinity76 commented Mar 19, 2024

command is not always available in shell_exec() , observed on Ubuntu-24.04-beta + PHP8.3
see #613 for details

command is not always available in shell_exec()
see chrome-php#613 for details
src/AutoDiscover.php Outdated Show resolved Hide resolved
@enricodias
Copy link
Member

My PATH has 21 folders, that would result in 84 checks (21 x 4) in the worst case. I know that command -v probably does the same thing under the hood, but it feels so wrong to scan folders manually like this 😅

@divinity76
Copy link
Contributor Author

divinity76 commented Mar 19, 2024

My PATH has 21 folders, that would result in 84 checks (21 x 4) in the worst case. I know that command -v probably does the same thing under the hood, but it feels so wrong to scan folders manually like this 😅

yeah i know what you mean😅 i have 25 here (WSL Ubuntu 24.04-beta)

Benchmarking it on a AMD Ryzen 9 7950x (high-end 2022 system) Ubuntu 24.04-beta WSL:

$ sudo bash -c 'sync; echo 3 > /proc/sys/vm/drop_caches';
$ time bash -c ' command -v google-chrome chromium-browser chrome chromium'
/usr/bin/chromium-browser
/snap/bin/chromium

real    0m0.024s
user    0m0.003s
sys     0m0.000s
$ time bash -c ' command -v google-chrome chromium-browser chrome chromium'
/usr/bin/chromium-browser
/snap/bin/chromium

real    0m0.022s
user    0m0.003s
sys     0m0.000s
$ sudo bash -c 'sync; echo 3 > /proc/sys/vm/drop_caches';
$ time php -r '                                          
$a=explode(PATH_SEPARATOR, getenv("PATH"));
var_dump(count($a));
$names=["google-chrome","chromium-browser","chrome","chromium"];
foreach($a as $dir){
  foreach($names as $name){
    $file=$dir.DIRECTORY_SEPARATOR.$name;
    if(file_exists($file) && is_executable($file)){
      echo $file."\n";
    }
  }
}
';
Command line code:3:
int(25)
/usr/bin/chromium-browser
/bin/chromium-browser
/snap/bin/chromium

real    0m0.097s
user    0m0.015s
sys     0m0.007s
$ time php -r '
$a=explode(PATH_SEPARATOR, getenv("PATH"));
var_dump(count($a));
$names=["google-chrome","chromium-browser","chrome","chromium"];
foreach($a as $dir){
  foreach($names as $name){
    $file=$dir.DIRECTORY_SEPARATOR.$name;
    if(file_exists($file) && is_executable($file)){
      echo $file."\n";
    }
  }
}
';
Command line code:3:
int(25)
/usr/bin/chromium-browser
/bin/chromium-browser
/snap/bin/chromium

real    0m0.041s
user    0m0.015s
sys     0m0.000s

(php does more work than command here because php does not bail, it simulates absolutely worst case scenario, command does not)

doing the same on a Intel i7-8565U, a 2018 15watt low-power laptop cpu running on battery in power-saving mode, on WSL Ubuntu 24.04-beta:

hans@DESKTOP-2LHJILI:~$ sudo bash -c 'sync; echo 3 > /proc/sys/vm/drop_caches';
hans@DESKTOP-2LHJILI:~$ time bash -c ' command -v google-chrome chromium-browser chrome chromium'
/usr/bin/chromium-browser
/snap/bin/chromium

real    0m0.820s
user    0m0.001s
sys     0m0.050s
hans@DESKTOP-2LHJILI:~$ time bash -c ' command -v google-chrome chromium-browser chrome chromium'
/usr/bin/chromium-browser
/snap/bin/chromium

real    0m0.476s
user    0m0.001s
sys     0m0.027s
hans@DESKTOP-2LHJILI:~$ sudo bash -c 'sync; echo 3 > /proc/sys/vm/drop_caches';
hans@DESKTOP-2LHJILI:~$ time php -r '
$a=explode(PATH_SEPARATOR, getenv("PATH"));
var_dump(count($a));
$names=["google-chrome","chromium-browser","chrome","chromium"];
foreach($a as $dir){
  foreach($names as $name){
    $file=$dir.DIRECTORY_SEPARATOR.$name;
    if(file_exists($file) && is_executable($file)){
      echo $file."\n";
    }
  }
}
';
int(32)
/usr/bin/chromium-browser
/bin/chromium-browser
/snap/bin/chromium

real    0m1.168s
user    0m0.062s
sys     0m0.076s
hans@DESKTOP-2LHJILI:~$ time php -r '
$a=explode(PATH_SEPARATOR, getenv("PATH"));
var_dump(count($a));
$names=["google-chrome","chromium-browser","chrome","chromium"];
foreach($a as $dir){
  foreach($names as $name){
    $file=$dir.DIRECTORY_SEPARATOR.$name;
    if(file_exists($file) && is_executable($file)){
      echo $file."\n";
    }
  }
}
';
int(32)
/usr/bin/chromium-browser
/bin/chromium-browser
/snap/bin/chromium

real    0m0.617s
user    0m0.030s
sys     0m0.030s

🤔 i think servers will do just fine. laptop users should specify chrome path to avoid the delay

@divinity76
Copy link
Contributor Author

could be cached, but then we'd need to consider cache invalidation and cache poisoning 🤔

@enricodias
Copy link
Member

enricodias commented Mar 20, 2024

The performance will always be ok since command do the same thing, it just feels wrong 😅. The code itself seems to work. If I have time in the next few days I'll see if there is any other better way of doing this.

Php already has the stat cache. It won't do anything in this case but it's still not an issue.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Mar 20, 2024

I'm not really sure we should be making auto-discover that complicated. The point is to detect the most common places, only. I feel like people should be passing in the binary path explicitly, otherwise, or implementing this kind of logic in their own app.

@divinity76
Copy link
Contributor Author

divinity76 commented Mar 21, 2024

@GrahamCampbell so you're basically proposing

$commonLocations = array(
"/usr/bin/chromium-browser",
"/bin/chromium-browser",
"/snap/bin/chromium",
(...)
);
ForEach($commonLocations as (...)

?

@divinity76 divinity76 closed this Mar 21, 2024
@divinity76 divinity76 reopened this Mar 21, 2024
@enricodias
Copy link
Member

I couldn't think of any better way to implement this and I recreating the command behaviour seems out of the scope of this project, but if you manage to convince Graham I'm ok with it.

@GrahamCampbell
Copy link
Member

I'm not going to go ahead with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants