DOMNodeList::item may return DOMElement. #11339
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* PHP-8.1: [ci skip] UPDATE NEWS
* PHP-8.2: [ci skip] UPDATE NEWS
Commit d835de1 added support for AVX2 in hash table initialization code. The same kind of code also occurs for HT_HASH_RESET. However, this place was forgotten in that patch. That is unfortunate, because a loop is just when there may be the most benefit from this SIMD sequence. Furthermore, the NEON special handling exists in the initialization code but is also missing from HT_HASH_RESET, so add this as well.
…onversion code for automatic encoding detection For mb_parse_str, when mbstring.http_input (INI parameter) is a list of multiple possible text encodings (which is not the case by default), this new implementation is about 25% faster. When mbstring.http_input is a single value, then nothing is changed. (No automatic encoding detection is done in that case.)
I checked a simple Laravel CRUD application's home page under Callgrind
and found that the line:
char resolved_path[MAXPATHLEN] = {0};
took up about 0.95% of the spent instruction count.
This is because when opcache revalidates the timestamps, it has to go
through the function virtual_file_ex() which contains that line. That
line will memset 4096 bytes on my system to all zeroes. This is bad for
the data cache and for the runtime.
I found that this memsetting is unnecessary in most cases, and that
we can fix the one remaining case:
* Lines 1020-1027 don't do anything with resolved_path, so that's okay.
* Lines 1033-1098:
- The !IS_ABSOLUTE_PATH branch will always result in a memcpy from
path to resolved_path (+ sometimes an offset) with the total copied
amount equal to path_length+1, so that includes a NUL byte.
- The else branch either takes the WIN32 path or the non-WIN32 path.
° WIN32: There's a copy from path+2 with length path_length-1.
Note that we chop off the first 2 bytes, so this also
includes the NUL byte.
° Non-WIN32: Copies path_length+1 bytes, so that includes a NUL byte.
At this point we know that resolved_path ends in a NUL byte. Going
further in the code:
* Lines 1100-1106 don't write to resolved_path, so no NUL byte is removed.
* Lines 1108-1136:
- The IS_UNC_PATH branch:
° Lines 1111-1112 don't overwrite the NUL byte, because we know the
path length is at least 2 due to the IS_UNC_PATH check.
° Both while loops uppercase the path until a slash is found. If a
NUL byte was found then it jumps to verify. Therefore, no NUL byte
can be overwritten. Furthermore, Lines 1121 and 1129 cannot
overwrite a NUL byte because the check at lines 1115 and 1123
would've jumped to verify when a NUL byte would be encountered.
Therefore, the IS_UNC_PATH branch cannot overwrite a NUL byte, so
the NUL byte we know we already got stays in place.
- The else branch:
° We know the path length is at least 2 due to IS_ABSOLUTE_PATH.
That means the earliest NUL byte can be at index 2, which can be
overwritten on line 1133. We fix this by adding one byte write if
the length is 2.
All uses of resolved_path in lines 1139-1141 have a NUL byte at the end
now.
Lines 1154-1164 do a bunch of post-processing but line 1164 will make
sure resolved_path still ends in a NUL byte.
So therefore I propose to remove the huge memset, and add a single byte
write in that one else branch I mentioned earlier.
Looking at Callgrind, the instruction count before this patch for 200
requests is 14,264,569,942; and after the patch it's 14,129,358,195
(averaged over a handful of runs).
* PHP-8.1: Fix test bug60120.phpt
* PHP-8.2: Fix test bug60120.phpt
Change whether to inline XXH3_hashLong_withSecret to a config option Ref: Cyan4973/xxHash@ace22bd Signed-off-by: Mingli Yu <mingli.yu@windriver.com> Closes phpGH-11062.
* PHP-8.1: [skip ci] Skip bug45161.phpt on Windows
* PHP-8.2: [skip ci] Skip bug45161.phpt on Windows
* PHP-8.1: [skip ci] Fix Slack notification
* PHP-8.2: [skip ci] Fix Slack notification
Otherwise the job will just fail due to missing ssh keys
When using ZEND_NORMALIZE_BOOL(a - b) where a and b are doubles, this generates the following instruction sequence on x64: subsd xmm0, xmm1 pxor xmm1, xmm1 comisd xmm0, xmm1 ... whereas if we use ZEND_THREEWAY_COMPARE we get two instructions less: ucomisd xmm0, xmm1 The only difference is that the threeway compare uses *u*comisd instead of comisd. The difference is that it will cause a FP signal if a signaling NAN is used, but as far as I'm aware this doesn't matter for our use case. Similarly, the amount of instructions on AArch64 is also quite a bit lower for this code compared to the old code. ** Results ** Using the benchmark https://gist.github.com/nielsdos/b36517d81a1af74d96baa3576c2b70df I used hyperfine: hyperfine --runs 25 --warmup 3 './sapi/cli/php sort_double.php' No extensions such as opcache used during benchmarking. BEFORE THIS PATCH ----------------- Time (mean ± σ): 255.5 ms ± 2.2 ms [User: 251.0 ms, System: 2.5 ms] Range (min … max): 251.5 ms … 260.7 ms 25 runs AFTER THIS PATCH ---------------- Time (mean ± σ): 236.2 ms ± 2.8 ms [User: 228.9 ms, System: 5.0 ms] Range (min … max): 231.5 ms … 242.7 ms 25 runs
We're not in pull_request-context, of course.
* PHP-8.1: Fix phpGH-11028: Heap Buffer Overflow in zval_undefined_cv.
* PHP-8.2: Fix phpGH-11028: Heap Buffer Overflow in zval_undefined_cv.
* Use 50 runs and calculate mean * Don't validate timestamps * Don't profile PHP startup and shutdown in cgi with valgrind
* PHP-8.2: [skip ci] Fix release date of PHP 8.2.7
* PHP-8.1: Access violation when ALLOC_FALLBACK fixed
* PHP-8.2: Access violation when ALLOC_FALLBACK fixed
* PHP-8.1: Fix phpGH-11288 and phpGH-11289 and phpGH-11290 and phpGH-9142: DOMExceptions and segfaults with replaceWith
* PHP-8.2: Fix phpGH-11288 and phpGH-11289 and phpGH-11290 and phpGH-9142: DOMExceptions and segfaults with replaceWith
…memory=1 The function repeatedly calls mprotect() which is extremely slow. In our community build, the Laravel tests went from ~6 minutes to ~4 hours. This issue only occurs with opcache.protect_memory=1. Closes phpGH-11323
* PHP-8.2: Fix zend_jit_stop_counter_handlers() performance issues with protect_memory=1
* PHP-8.1: [skip ci] Fix race condition in readline test
* PHP-8.2: [skip ci] Fix race condition in readline test
Array literals will constant evaluate their elements. These can include assignments, even though these are not valid constant expressions. The lhs of assignments can be a list() element (or []) which is parsed as an array with a special flag.
This reverts commit 1c733c8. Fixes phpGH-11320
* PHP-8.1: [skip ci] Add more patterns to run-tests.php retry list
* PHP-8.2: [skip ci] Add more patterns to run-tests.php retry list
These values are only ever bools, store them as bools. Reduces the size from 40 bytes to 16 bytes on my system.
* PHP-8.1: Fix phpGH-10234: Setting DOMAttr::textContent results in an empty attribute value
* PHP-8.2: Fix phpGH-10234: Setting DOMAttr::textContent results in an empty attribute value
Not explicitly documenting the possibility of returning DOMElement causes the Intelephense linter (a popular PHP linter with ~9 million downloads: https://marketplace.visualstudio.com/items?itemName=bmewburn.vscode-intelephense-client ) to think this code is bad: $xp->query("whatever")->item(0)->getAttribute("foo"); because DOMNode does not have getAttribute (while DOMElement does). documenting the DOMElement return type should fix Intelephense's linter.
|
Please target branch PHP-8.1 instead of master, since this is a bugfix. |
|
Wow, using the github.com interface to switch branch definitely didn't work. |
|
Yeah, you'd have to rebase as well such that you get rid of all the additional commits. But that's a bit cumbersome and in this case recreating it is easier indeed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
39 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Not explicitly documenting the possibility of returning DOMElement causes the Intelephense linter (a popular PHP linter with ~9 million downloads: https://marketplace.visualstudio.com/items?itemName=bmewburn.vscode-intelephense-client ) to think this code is bad:
because DOMNode does not have getAttribute (while DOMElement does). documenting the DOMElement return type should fix Intelephense's linter.