Zend: Preallocate error buffer with capacity tracking #20565
Conversation
|
It's often a good idea to choose a factor of 1.5, there's a mathematical reason behind it (https://github.com/facebook/folly/blob/main/folly/docs/FBVector.md#memory-handling). |
|
yes will try ; even though usage and growth might not be that as critical as your (nice) link contexts. |
Interesting, we currently extend arrays by a factor of 2, I wonder what would happen if we do 1.5 instead. |
Won't work because the hash masking depends on it being a power of 2. |
|
Can we get a clearer PR title? |
devnexen
changed the title
Zend/zend.c
Outdated
| * Use pow2 realloc if it becomes a problem. */ | ||
| EG(num_errors)++; | ||
| EG(errors) = erealloc(EG(errors), sizeof(zend_error_info*) * EG(num_errors)); | ||
| // pondering if uint32_t is more appropriate but would need to align the allocation size then |
There was a problem hiding this comment.
Storing the size like this is very ugly in my opinion. What you could do is make a struct with a size field and a flexible array member at the end.
devnexen
force-pushed
the
zend_little_upd
branch
from
d9f84e7 to
c6bed1f
Compare
devnexen
force-pushed
the
zend_little_upd
branch
8 times, most recently
from
f38b0e3 to
078130f
Compare
devnexen
force-pushed
the
zend_little_upd
branch
7 times, most recently
from
21c2fdc to
5111ca5
Compare
devnexen
changed the title
devnexen
changed the title
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
When dealing with nested fields in module globals, we normally use the EG(errors)->size etc syntax instead of EG(errors->size). Please change this.
I'm not sure how I feel about unconditionally allocating a buffer in the executor globals. I understand this is to allow unconditional access to fields like EG(errors)->size ? I see previously it was always safe to access EG(num_errors).
Zend/zend_globals.h
Outdated
| HashTable callable_convert_cache; | ||
|
|
||
| void *reserved[ZEND_MAX_RESERVED_RESOURCES]; | ||
| struct zend_err_buf *errors; |
There was a problem hiding this comment.
I don't believe this should be at the end, please place this under record_errors
There was a problem hiding this comment.
no it does not need to.
Zend/zend.c
Outdated
| EG(errors)[EG(num_errors)-1] = info; | ||
| EG(errors->size)++; | ||
| if (EG(errors->size) >= EG(errors->capacity)) { | ||
| // not sure we can get high number of errors so safe `might be` over cautious here |
|
|
||
| static void zend_init_errors_buf(zend_executor_globals *eg) | ||
| { | ||
| eg->errors = pemalloc(ZEND_ERR_BUF_SIZE(2), true); |
There was a problem hiding this comment.
well it needs to live long enough. I m not yet that familiar with Zend engine (why I m doing this PR) though. I ll try in non persistent mode just in case..
Yes this is the reason, remember we re dealing with a "flexible" array now. |
Replace separate num_errors/errors fields with a single struct containing size, capacity, and a flexible array. The buffer grows by 50% when needed instead of reallocating on every recorded error. - Add ZEND_ERR_BUF_SIZE macro and zend_init_errors_buf() helper - Allocate error buffer in zend_startup for NTS builds - Remove redundant NULL checks since buffer is always allocated
devnexen
force-pushed
the
zend_little_upd
branch
3 times, most recently
from
ecc36fc to
3ac9616
Compare
4 participants
Replace separate num_errors/errors fields with a single struct containing size, capacity, and a flexible array. The buffer grows by 50% when needed instead of reallocating on every recorded error.