Skip to content

Improve browscap get_browser performance #12651

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

Merged
merged 19 commits into from
Nov 11, 2023
Merged

Conversation

nielsdos
Copy link
Member

This set of patches improves the browscap support performance. The changes are split in atomic commits to make review easier. Sometimes the commits also contain a description. The most important change here is moving away from regexes and using a specialised wildcard matching algorithm (best case O(n), worst case O(n²)). This gives the most speed-up especially as compilation and conversion to regexes is prevented. The algorithm has the same execution time complexity as the regex.

With the following 3 test scenarios:

(1) Unknown data lookup

for ($i = 0; $i < 10000; $i++) {
    get_browser("Unknown Browser 1.0");
}

(2) Test all known user agents we test for

$user_agents = file('ext/standard/tests/misc/user_agents.txt', FILE_IGNORE_NEW_LINES);
foreach ($user_agents as $ua) {
    get_browser($ua, true);
}

(3) Result construction

for ($i = 0; $i < 200000; $i++) {
    get_browser("Chromium 123.0");
}

Benchmark results

For (1):

Benchmark 1: ./sapi/cli/php bench.php
  Time (mean ± σ):     331.0 ms ±   3.9 ms    [User: 325.9 ms, System: 4.2 ms]
  Range (min … max):   325.7 ms … 339.3 ms    10 runs
 
Benchmark 2: ./sapi/cli/php_old bench.php
  Time (mean ± σ):     478.2 ms ±   4.3 ms    [User: 472.6 ms, System: 4.2 ms]
  Range (min … max):   472.9 ms … 486.1 ms    10 runs
 
Summary
  ./sapi/cli/php bench.php ran
    1.44 ± 0.02 times faster than ./sapi/cli/php_old bench.php

For (2):

Benchmark 1: ./sapi/cli/php bench.php
  Time (mean ± σ):     243.6 ms ±   3.9 ms    [User: 237.2 ms, System: 5.8 ms]
  Range (min … max):   239.5 ms … 252.2 ms    12 runs
 
Benchmark 2: ./sapi/cli/php_old bench.php
  Time (mean ± σ):     618.5 ms ±   4.1 ms    [User: 603.4 ms, System: 12.7 ms]
  Range (min … max):   614.2 ms … 626.4 ms    10 runs
 
Summary
  ./sapi/cli/php bench.php ran
    2.54 ± 0.04 times faster than ./sapi/cli/php_old bench.php

For (3):

Benchmark 1: ./sapi/cli/php bench.php
  Time (mean ± σ):      89.3 ms ±   4.6 ms    [User: 85.4 ms, System: 3.5 ms]
  Range (min … max):    85.5 ms … 101.5 ms    33 runs
 
Benchmark 2: ./sapi/cli/php_old bench.php
  Time (mean ± σ):     144.9 ms ±   4.7 ms    [User: 139.4 ms, System: 5.1 ms]
  Range (min … max):   140.8 ms … 162.2 ms    20 runs
 
Summary
  ./sapi/cli/php bench.php ran
    1.62 ± 0.10 times faster than ./sapi/cli/php_old bench.php

We're already doing a character-wise loop after lowering, so just lower
it character by character instead of looping over it twice and
allocating memory.
This avoids compiling, allocating and caching regexes and should run in
the same complexity.
The function prologue and epilogue have a stupidly high overhead for
those 2 simple checks at the start.

We can't always-inline the reg_compare function because it contains
alloca, and the alloca is really important for performance.
Instead, move those cheap checks to the call site.
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM other than the one comment about explaining why the checks are not in the compare function :)

ZSTR_VAL(lookup_browser_name), entry->prefix_len,
ZSTR_VAL(entry->pattern), entry->prefix_len) != 0) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment explaning why this is not in the comparison function so people know why

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, good point. Otherwise someone else in the future may move them again.

@nielsdos nielsdos merged commit f150f99 into php:master Nov 11, 2023

res = zend_string_alloc(browscap_compute_regex_len(pattern), persistent);
t = ZSTR_VAL(res);

lc_pattern = do_alloca(ZSTR_LEN(pattern) + 1, use_heap);
zend_str_tolower_copy(lc_pattern, ZSTR_VAL(pattern), ZSTR_LEN(pattern));

t[j++] = '~';
t[j++] = '^';

for (i = 0; i < ZSTR_LEN(pattern); i++, j++) {

Choose a reason for hiding this comment

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

isn't ZSTR_LEN will call on every iteration

Copy link
Member

Choose a reason for hiding this comment

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

It is a macro that just points to the size field of the zend_string struct.

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.

3 participants