Skip to content

Proposal: Add final class Vector to PHP #7488

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

Proposal: Add final class Vector to PHP #7488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a224a89
Proposal: Add `final class Vector` to PHP
TysonAndre Sep 12, 2021
460b1eb
Support `$vector[] = $value;`
TysonAndre Sep 12, 2021
ebf2590
Fix strange bug seen with '-O2', '--with-valgrind', and valgrind test
TysonAndre Sep 12, 2021
5431aba
Add Vector::map/filter implementations
TysonAndre Sep 15, 2021
b67095c
WIP reduce public api
TysonAndre Sep 23, 2021
f3cea21
wip
TysonAndre Sep 23, 2021
1ac4c83
Update API after RFC discussion
TysonAndre Sep 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 1 addition & 16 deletions Zend/zend_string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,22 +148,7 @@ static zend_always_inline zend_string *zend_interned_string_ht_lookup_ex(zend_ul

static zend_always_inline zend_string *zend_interned_string_ht_lookup(zend_string *str, HashTable *interned_strings)
{
zend_ulong h = ZSTR_H(str);
uint32_t nIndex;
uint32_t idx;
Bucket *p;

nIndex = h | interned_strings->nTableMask;
idx = HT_HASH(interned_strings, nIndex);
while (idx != HT_INVALID_IDX) {
p = HT_HASH_TO_BUCKET(interned_strings, idx);
if ((p->h == h) && zend_string_equal_content(p->key, str)) {
return p->key;
}
idx = Z_NEXT(p->val);
}

return NULL;
return zend_interned_string_ht_lookup_ex(ZSTR_H(str), ZSTR_VAL(str), ZSTR_LEN(str), interned_strings);
Copy link
Contributor Author

@TysonAndre TysonAndre Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@nikic FYI - I wonder if you're able to reproduce the test failure in the prior commit (in gcc 9.3.0-17ubuntu1~20.04) for make test TESTS='ext/spl/tests/Vector/aggregate.phpt -m --show-mem --show-diff (it segfaults in idx = Z_NEXT(p->val); when compiling the code and interning the strings, not when running it). It seems almost definitely related to running the code under valgrind with -O2 [-g]. Even after git clean -fdx and recompiling it's still an issue

  • Oddly, running valgrind tests in docker in other OSes (e.g. dockerhub php:8.1.0RC1 with gcc (Debian 10.2.1-6) 10.2.1 20210110) don't have this issue.
  • When I add fprintf(stderr statements, the issue goes away

(The segfault doesn't happen with gdb, or even gdb with USE_ZEND_ALLOC=0 - it's baffling.)

  • If you are able to reproduce this, it's probably a symptom of a larger issue that may affect other php 8.1/8.2 users using the same compiler
  • If you aren't, it still may be a good idea to avoid (1) repeating code and (2) redundant loads of ZSTR_LEN(str) from memory into registers if gcc can't infer that the assembly function zend_string_equal_val doesn't modify memory in the assembly block, and needs to scan over multiple buckets (probably not redundant if the hash is the same, hash collision chance is extremely low)

Options used to build in linux mint 20.2: export CFLAGS='-O2 -g'; ./buildconf --force; ./configure --disable-all --with-zlib --with-zip --enable-phar --with-bz2 --enable-opcache --with-readline --enable-tokenizer --prefix=$HOME/php-8.2-unoptimized-nts-vector-install --with-valgrind; make -j8

}

/* This function might be not thread safe at least because it would update the
Expand Down
4 changes: 2 additions & 2 deletions ext/spl/config.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
PHP_NEW_EXTENSION(spl, php_spl.c spl_functions.c spl_iterators.c spl_array.c spl_directory.c spl_exceptions.c spl_observer.c spl_dllist.c spl_heap.c spl_fixedarray.c, no,, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1)
PHP_INSTALL_HEADERS([ext/spl], [php_spl.h spl_array.h spl_directory.h spl_engine.h spl_exceptions.h spl_functions.h spl_iterators.h spl_observer.h spl_dllist.h spl_heap.h spl_fixedarray.h])
PHP_NEW_EXTENSION(spl, php_spl.c spl_functions.c spl_iterators.c spl_array.c spl_directory.c spl_exceptions.c spl_observer.c spl_dllist.c spl_heap.c spl_fixedarray.c spl_vector.c, no,, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1)
PHP_INSTALL_HEADERS([ext/spl], [php_spl.h spl_array.h spl_directory.h spl_engine.h spl_exceptions.h spl_functions.h spl_iterators.h spl_observer.h spl_dllist.h spl_heap.h spl_fixedarray.h spl_vector.h])
PHP_ADD_EXTENSION_DEP(spl, pcre, true)
PHP_ADD_EXTENSION_DEP(spl, standard, true)
PHP_ADD_EXTENSION_DEP(spl, json)
4 changes: 2 additions & 2 deletions ext/spl/config.w32
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// vim:ft=javascript

EXTENSION("spl", "php_spl.c spl_functions.c spl_iterators.c spl_array.c spl_directory.c spl_exceptions.c spl_observer.c spl_dllist.c spl_heap.c spl_fixedarray.c", false /*never shared */, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
EXTENSION("spl", "php_spl.c spl_functions.c spl_iterators.c spl_array.c spl_directory.c spl_exceptions.c spl_observer.c spl_dllist.c spl_heap.c spl_fixedarray.c spl_vector.c", false /*never shared */, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
PHP_SPL="yes";
PHP_INSTALL_HEADERS("ext/spl", "php_spl.h spl_array.h spl_directory.h spl_engine.h spl_exceptions.h spl_functions.h spl_iterators.h spl_observer.h spl_dllist.h spl_heap.h spl_fixedarray.h");
PHP_INSTALL_HEADERS("ext/spl", "php_spl.h spl_array.h spl_directory.h spl_engine.h spl_exceptions.h spl_functions.h spl_iterators.h spl_observer.h spl_dllist.h spl_heap.h spl_fixedarray.h spl_vector.h");
2 changes: 2 additions & 0 deletions ext/spl/php_spl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "spl_observer.h"
#include "spl_dllist.h"
#include "spl_fixedarray.h"
#include "spl_vector.h"
#include "spl_heap.h"
#include "zend_exceptions.h"
#include "zend_interfaces.h"
Expand Down Expand Up @@ -708,6 +709,7 @@ PHP_MINIT_FUNCTION(spl)
PHP_MINIT(spl_dllist)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(spl_heap)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(spl_fixedarray)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(spl_vector)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(spl_observer)(INIT_FUNC_ARGS_PASSTHRU);

return SUCCESS;
Expand Down
32 changes: 1 addition & 31 deletions ext/spl/spl_fixedarray.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "spl_fixedarray.h"
#include "spl_exceptions.h"
#include "spl_iterators.h"
#include "spl_util.h"
#include "ext/json/php_json.h"

zend_object_handlers spl_handler_SplFixedArray;
Expand Down Expand Up @@ -310,37 +311,6 @@ static zend_object *spl_fixedarray_object_clone(zend_object *old_object)
return new_object;
}

static zend_long spl_offset_convert_to_long(zval *offset) /* {{{ */
{
try_again:
switch (Z_TYPE_P(offset)) {
case IS_STRING: {
zend_ulong index;
if (ZEND_HANDLE_NUMERIC(Z_STR_P(offset), index)) {
return (zend_long) index;
}
break;
}
case IS_DOUBLE:
return zend_dval_to_lval_safe(Z_DVAL_P(offset));
case IS_LONG:
return Z_LVAL_P(offset);
case IS_FALSE:
return 0;
case IS_TRUE:
return 1;
case IS_REFERENCE:
offset = Z_REFVAL_P(offset);
goto try_again;
case IS_RESOURCE:
zend_use_resource_as_offset(offset);
return Z_RES_HANDLE_P(offset);
}

zend_type_error("Illegal offset type");
return 0;
}

static zval *spl_fixedarray_object_read_dimension_helper(spl_fixedarray_object *intern, zval *offset)
{
zend_long index;
Expand Down
37 changes: 37 additions & 0 deletions ext/spl/spl_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#ifndef SPL_UTIL
#define SPL_UTIL

#include "zend_types.h"

static zend_long spl_offset_convert_to_long(zval *offset) /* {{{ */
{
try_again:
switch (Z_TYPE_P(offset)) {
case IS_STRING: {
zend_ulong index;
if (ZEND_HANDLE_NUMERIC(Z_STR_P(offset), index)) {
return (zend_long) index;
}
break;
}
case IS_DOUBLE:
return zend_dval_to_lval_safe(Z_DVAL_P(offset));
case IS_LONG:
return Z_LVAL_P(offset);
case IS_FALSE:
return 0;
case IS_TRUE:
return 1;
case IS_REFERENCE:
offset = Z_REFVAL_P(offset);
goto try_again;
case IS_RESOURCE:
zend_use_resource_as_offset(offset);
return Z_RES_HANDLE_P(offset);
}

zend_type_error("Illegal offset type");
return 0;
}

#endif
Loading