-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
…g temporary memory allocations
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 saves additional checks.
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.
There was a problem hiding this 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; | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
||
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++) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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
(2) Test all known user agents we test for
(3) Result construction
Benchmark results
For (1):
For (2):
For (3):