Skip to content

Always use CE_CACHE, remove TYPE_HAS_CE #7336

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

Merged
merged 4 commits into from
Aug 11, 2021
Merged

Always use CE_CACHE, remove TYPE_HAS_CE #7336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58d6a20
Always use CE_CACHE, remove TYPE_HAS_CE
nikic Aug 3, 2021
0a33dd6
Reset startup_done variable
nikic Aug 10, 2021
ed6bc8f
Simplify get_class_from_type() and users
nikic Aug 10, 2021
2f5faea
Skip ce cache copying code if already present
nikic Aug 10, 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
4 changes: 1 addition & 3 deletions Zend/Optimizer/zend_inference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2269,9 +2269,7 @@ static uint32_t zend_convert_type(const zend_script *script, zend_type type, zen
if (pce) {
/* As we only have space to store one CE,
* we use a plain object type for class unions. */
if (ZEND_TYPE_HAS_CE(type)) {
*pce = ZEND_TYPE_CE(type);
} else if (ZEND_TYPE_HAS_NAME(type)) {
if (ZEND_TYPE_HAS_NAME(type)) {
zend_string *lcname = zend_string_tolower(ZEND_TYPE_NAME(type));
*pce = zend_optimizer_get_class_entry(script, lcname);
zend_string_release_ex(lcname, 0);
Expand Down
4 changes: 0 additions & 4 deletions Zend/tests/objects_008.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
--TEST--
method overloading with different method signature
--SKIPIF--
<?php
if (getenv('SKIP_PRELOAD')) die('xfail Difference in class name casing');
?>
--FILE--
<?php

Expand Down
64 changes: 29 additions & 35 deletions Zend/zend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include "Optimizer/zend_optimizer.h"

static size_t global_map_ptr_last = 0;
static bool startup_done = false;

#ifdef ZTS
ZEND_API int compiler_globals_id;
Expand Down Expand Up @@ -1018,40 +1019,6 @@ void zend_register_standard_ini_entries(void) /* {{{ */
}
/* }}} */

static zend_class_entry *resolve_type_name(zend_string *type_name) {
zend_string *lc_type_name = zend_string_tolower(type_name);
zend_class_entry *ce = zend_hash_find_ptr(CG(class_table), lc_type_name);

ZEND_ASSERT(ce && ce->type == ZEND_INTERNAL_CLASS);
zend_string_release(lc_type_name);
return ce;
}

static void zend_resolve_property_types(void) /* {{{ */
{
zend_class_entry *ce;
zend_property_info *prop_info;

ZEND_HASH_FOREACH_PTR(CG(class_table), ce) {
if (ce->type != ZEND_INTERNAL_CLASS) {
continue;
}

if (UNEXPECTED(ZEND_CLASS_HAS_TYPE_HINTS(ce))) {
ZEND_HASH_FOREACH_PTR(&ce->properties_info, prop_info) {
zend_type *single_type;
ZEND_TYPE_FOREACH(prop_info->type, single_type) {
if (ZEND_TYPE_HAS_NAME(*single_type)) {
zend_string *type_name = ZEND_TYPE_NAME(*single_type);
ZEND_TYPE_SET_CE(*single_type, resolve_type_name(type_name));
zend_string_release(type_name);
}
} ZEND_TYPE_FOREACH_END();
} ZEND_HASH_FOREACH_END();
}
} ZEND_HASH_FOREACH_END();
}
/* }}} */

/* Unlink the global (r/o) copies of the class, function and constant tables,
* and use a fresh r/w copy for the startup thread
Expand All @@ -1065,7 +1032,7 @@ zend_result zend_post_startup(void) /* {{{ */
zend_executor_globals *executor_globals = ts_resource(executor_globals_id);
#endif

zend_resolve_property_types();
startup_done = true;

if (zend_post_startup_cb) {
zend_result (*cb)(void) = zend_post_startup_cb;
Expand Down Expand Up @@ -1164,6 +1131,7 @@ void zend_shutdown(void) /* {{{ */
zend_destroy_rsrc_list_dtors();

zend_optimizer_shutdown();
startup_done = false;
}
/* }}} */

Expand Down Expand Up @@ -1892,3 +1860,29 @@ ZEND_API void zend_map_ptr_extend(size_t last)
CG(map_ptr_last) = last;
}
}

ZEND_API void zend_alloc_ce_cache(zend_string *type_name)
{
if (ZSTR_HAS_CE_CACHE(type_name) || !ZSTR_IS_INTERNED(type_name)) {
return;
}

if ((GC_FLAGS(type_name) & IS_STR_PERMANENT) && startup_done) {
Comment on lines +1866 to +1870
Copy link
Member

Choose a reason for hiding this comment

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

This means, that we won't allocate CE cache slots during requests if opcache active. Right?
This will affect eval()'ed code, opcache.blacklisted files and situation when opcache is full.

Copy link
Member Author

@nikic nikic Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this is the key tradeoff with this patch. There are cases where we can't allocate CE_CACHE (eval with opcache, permanent strings during request without opcache), so property types will be uncached (arg types still have separate cache slot).

I didn't have any simple ideas on how to address this. One thought I had is that we could change the map_ptr mechanism to not work using "ptr or offset", but to have two different map_ptr_base for permanent and per-request and always use offset. That is *(map_ptr_base[ptr & 1] + ptr) as the access method. Per-request slots would then be allocated in the map_ptr area rather than on CG arena.

This would allow allocating new per-request slots without clashing with new permanent slots allocated by opcache. However, just that isn't enough as well, as with opcache there are no per-request interned strings at all, and we can't use the refcount as storage for non-interned strings.

Do you have any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

It's possible to grow map_ptr vector in both directions having the single biased base. e.g. positive offsets for permanent slots and negative for per-request slots. anyway, this is not for 8.1

/* Don't allocate slot on permanent interned string outside module startup.
* The cache slot would no longer be valid on the next request. */
return;
}

if (zend_string_equals_literal_ci(type_name, "self")
|| zend_string_equals_literal_ci(type_name, "parent")) {
Comment on lines +1876 to +1877
Copy link
Member

Choose a reason for hiding this comment

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

It may make sense to add self and parent into ZEND_KNOWN_STRINGS and compare just pointers.

return;
}

/* We use the refcount to keep map_ptr of corresponding type */
uint32_t ret;
do {
ret = (uint32_t)(uintptr_t)zend_map_ptr_new();
} while (ret <= 2);
GC_ADD_FLAGS(type_name, IS_STR_CLASS_NAME_MAP_PTR);
GC_SET_REFCOUNT(type_name, ret);
}
12 changes: 12 additions & 0 deletions Zend/zend_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3053,6 +3053,7 @@ static zend_class_entry *do_register_internal_class(zend_class_entry *orig_class

class_entry->type = ZEND_INTERNAL_CLASS;
zend_initialize_class_data(class_entry, 0);
zend_alloc_ce_cache(class_entry->name);
class_entry->ce_flags = orig_class_entry->ce_flags | ce_flags | ZEND_ACC_CONSTANTS_UPDATED | ZEND_ACC_LINKED | ZEND_ACC_RESOLVED_PARENT | ZEND_ACC_RESOLVED_INTERFACES;
class_entry->info.internal.module = EG(current_module);

Expand Down Expand Up @@ -4126,6 +4127,17 @@ ZEND_API zend_property_info *zend_declare_typed_property(zend_class_entry *ce, z
property_info->ce = ce;
property_info->type = type;

if (is_persistent_class(ce)) {
zend_type *single_type;
ZEND_TYPE_FOREACH(property_info->type, single_type) {
if (ZEND_TYPE_HAS_NAME(*single_type)) {
zend_string *name = zend_new_interned_string(ZEND_TYPE_NAME(*single_type));
ZEND_TYPE_SET_PTR(*single_type, name);
zend_alloc_ce_cache(name);
Comment on lines +4134 to +4136
Copy link
Member

Choose a reason for hiding this comment

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

This won't allocate new cache slot for eval()'ed code when opcache active (or when opcache is full).

}
} ZEND_TYPE_FOREACH_END();
}

zend_hash_update_ptr(&ce->properties_info, name, property_info);

return property_info;
Expand Down
45 changes: 10 additions & 35 deletions Zend/zend_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,43 +1202,13 @@ zend_string *zend_type_to_string_resolved(zend_type type, zend_class_entry *scop
zend_type *list_type;
bool is_intersection = ZEND_TYPE_IS_INTERSECTION(type);
ZEND_TYPE_LIST_FOREACH(ZEND_TYPE_LIST(type), list_type) {
if (ZEND_TYPE_HAS_CE(*list_type)) {
str = add_type_string(str, ZEND_TYPE_CE(*list_type)->name, is_intersection);
} else {
zend_string *name = ZEND_TYPE_NAME(*list_type);

if (ZSTR_HAS_CE_CACHE(name)
&& ZSTR_GET_CE_CACHE(name)) {
zend_class_entry *ce = ZSTR_GET_CE_CACHE(name);
if (ce->ce_flags & ZEND_ACC_ANON_CLASS) {
zend_string *tmp = zend_string_init(ZSTR_VAL(ce->name), strlen(ZSTR_VAL(ce->name)), 0);
str = add_type_string(str, tmp, is_intersection);
} else {
str = add_type_string(str, ce->name, is_intersection);
}
} else {
zend_string *resolved = resolve_class_name(name, scope);
str = add_type_string(str, resolved, is_intersection);
zend_string_release(resolved);
}
}
zend_string *name = ZEND_TYPE_NAME(*list_type);
zend_string *resolved = resolve_class_name(name, scope);
str = add_type_string(str, resolved, is_intersection);
zend_string_release(resolved);
} ZEND_TYPE_LIST_FOREACH_END();
} else if (ZEND_TYPE_HAS_NAME(type)) {
zend_string *name = ZEND_TYPE_NAME(type);

if (ZSTR_HAS_CE_CACHE(name)
&& ZSTR_GET_CE_CACHE(name)) {
zend_class_entry *ce = ZSTR_GET_CE_CACHE(name);
if (ce->ce_flags & ZEND_ACC_ANON_CLASS) {
str = zend_string_init(ZSTR_VAL(ce->name), strlen(ZSTR_VAL(ce->name)), 0);
} else {
str = zend_string_copy(ce->name);
}
} else {
str = resolve_class_name(name, scope);
}
} else if (ZEND_TYPE_HAS_CE(type)) {
str = zend_string_copy(ZEND_TYPE_CE(type)->name);
str = resolve_class_name(ZEND_TYPE_NAME(type), scope);
}

uint32_t type_mask = ZEND_TYPE_PURE_MASK(type);
Expand Down Expand Up @@ -6232,6 +6202,8 @@ static zend_type zend_compile_single_typename(zend_ast *ast)
}
}

class_name = zend_new_interned_string(class_name);
zend_alloc_ce_cache(class_name);
return (zend_type) ZEND_TYPE_INIT_CLASS(class_name, 0, 0);
}
}
Expand Down Expand Up @@ -7687,6 +7659,9 @@ void zend_compile_class_decl(znode *result, zend_ast *ast, bool toplevel) /* {{{
ce->type = ZEND_USER_CLASS;
ce->name = name;
zend_initialize_class_data(ce, 1);
if (!(decl->flags & ZEND_ACC_ANON_CLASS)) {
zend_alloc_ce_cache(ce->name);
}

if (CG(compiler_options) & ZEND_COMPILE_PRELOAD) {
ce->ce_flags |= ZEND_ACC_PRELOADED;
Expand Down
23 changes: 4 additions & 19 deletions Zend/zend_execute.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,11 +851,6 @@ ZEND_API ZEND_COLD void ZEND_FASTCALL zend_readonly_property_modification_error(

static zend_class_entry *resolve_single_class_type(zend_string *name, zend_class_entry *self_ce) {
if (zend_string_equals_literal_ci(name, "self")) {
/* We need to explicitly check for this here, to avoid updating the type in the trait and
* later using the wrong "self" when the trait is used in a class. */
if (UNEXPECTED((self_ce->ce_flags & ZEND_ACC_TRAIT) != 0)) {
return NULL;
}
return self_ce;
} else if (zend_string_equals_literal_ci(name, "parent")) {
return self_ce->parent;
Expand All @@ -866,26 +861,16 @@ static zend_class_entry *resolve_single_class_type(zend_string *name, zend_class

static zend_always_inline zend_class_entry *zend_ce_from_type(
zend_property_info *info, zend_type *type) {
if (UNEXPECTED(!ZEND_TYPE_HAS_NAME(*type))) {
ZEND_ASSERT(ZEND_TYPE_HAS_CE(*type));
return ZEND_TYPE_CE(*type);
}

ZEND_ASSERT(ZEND_TYPE_HAS_NAME(*type));
zend_string *name = ZEND_TYPE_NAME(*type);
zend_class_entry *ce;
if (ZSTR_HAS_CE_CACHE(name)) {
ce = ZSTR_GET_CE_CACHE(name);
zend_class_entry *ce = ZSTR_GET_CE_CACHE(name);
if (!ce) {
ce = zend_lookup_class_ex(name, NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD);
}
} else {
ce = resolve_single_class_type(name, info->ce);
if (ce && !(info->ce->ce_flags & ZEND_ACC_IMMUTABLE)) {
zend_string_release(name);
ZEND_TYPE_SET_CE(*type, ce);
}
return ce;
}
return ce;
return resolve_single_class_type(name, info->ce);
}

static bool zend_check_and_resolve_property_class_type(
Expand Down
34 changes: 9 additions & 25 deletions Zend/zend_inheritance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,9 +430,6 @@ static inheritance_status zend_is_intersection_subtype_of_class(

if (!proto_ce) proto_ce = lookup_class(proto_scope, proto_class_name);
fe_ce = lookup_class(fe_scope, fe_class_name);
} else if (ZEND_TYPE_HAS_CE(*single_type)) {
if (!proto_ce) proto_ce = lookup_class(proto_scope, proto_class_name);
fe_ce = ZEND_TYPE_CE(*single_type);
} else {
/* standard type in an intersection type is impossible,
* because it would be a fatal compile error */
Expand All @@ -456,8 +453,9 @@ static inheritance_status zend_is_intersection_subtype_of_class(

/* Check whether a single class proto type is a subtype of a potentially complex fe_type. */
static inheritance_status zend_is_class_subtype_of_type(
zend_class_entry *fe_scope, zend_string *fe_class_name, zend_class_entry *fe_ce,
zend_class_entry *fe_scope, zend_string *fe_class_name,
zend_class_entry *proto_scope, zend_type proto_type) {
zend_class_entry *fe_ce = NULL;
bool have_unresolved = 0;

/* If the parent has 'object' as a return type, any class satisfies the co-variant check */
Expand Down Expand Up @@ -503,9 +501,6 @@ static inheritance_status zend_is_class_subtype_of_type(

if (!fe_ce) fe_ce = lookup_class(fe_scope, fe_class_name);
proto_ce = lookup_class(proto_scope, proto_class_name);
} else if (ZEND_TYPE_HAS_CE(*single_type)) {
if (!fe_ce) fe_ce = lookup_class(fe_scope, fe_class_name);
proto_ce = ZEND_TYPE_CE(*single_type);
} else {
/* standard type */
ZEND_ASSERT(!is_intersection);
Expand Down Expand Up @@ -535,16 +530,10 @@ static inheritance_status zend_is_class_subtype_of_type(
return is_intersection ? INHERITANCE_SUCCESS : INHERITANCE_ERROR;
}

static zend_string *get_class_from_type(
zend_class_entry **ce, zend_class_entry *scope, zend_type single_type) {
static zend_string *get_class_from_type(zend_class_entry *scope, zend_type single_type) {
if (ZEND_TYPE_HAS_NAME(single_type)) {
*ce = NULL;
return resolve_class_name(scope, ZEND_TYPE_NAME(single_type));
}
if (ZEND_TYPE_HAS_CE(single_type)) {
*ce = ZEND_TYPE_CE(single_type);
return (*ce)->name;
}
return NULL;
}

Expand Down Expand Up @@ -617,14 +606,11 @@ static inheritance_status zend_perform_covariant_type_check(
if (proto_type_mask & (MAY_BE_OBJECT|MAY_BE_ITERABLE)) {
bool any_class = (proto_type_mask & MAY_BE_OBJECT) != 0;
ZEND_TYPE_FOREACH(fe_type, single_type) {
zend_class_entry *fe_ce;
zend_string *fe_class_name = get_class_from_type(&fe_ce, fe_scope, *single_type);
zend_string *fe_class_name = get_class_from_type(fe_scope, *single_type);
if (!fe_class_name) {
continue;
}
if (!fe_ce) {
fe_ce = lookup_class(fe_scope, fe_class_name);
}
zend_class_entry *fe_ce = lookup_class(fe_scope, fe_class_name);
if (fe_ce) {
if (any_class || unlinked_instanceof(fe_ce, zend_ce_traversable)) {
track_class_dependency(fe_ce, fe_class_name);
Expand All @@ -643,13 +629,12 @@ static inheritance_status zend_perform_covariant_type_check(
early_exit_status =
ZEND_TYPE_IS_INTERSECTION(proto_type) ? INHERITANCE_ERROR : INHERITANCE_SUCCESS;
ZEND_TYPE_FOREACH(proto_type, single_type) {
zend_class_entry *proto_ce;
zend_string *proto_class_name =
get_class_from_type(&proto_ce, proto_scope, *single_type);
zend_string *proto_class_name = get_class_from_type(proto_scope, *single_type);
if (!proto_class_name) {
continue;
}

zend_class_entry *proto_ce = NULL;
inheritance_status status = zend_is_intersection_subtype_of_class(
fe_scope, fe_type, proto_scope, proto_class_name, proto_ce);
if (status == early_exit_status) {
Expand All @@ -666,14 +651,13 @@ static inheritance_status zend_perform_covariant_type_check(
* whether proto_type is a union or intersection (only the inner check differs). */
early_exit_status = INHERITANCE_ERROR;
ZEND_TYPE_FOREACH(fe_type, single_type) {
zend_class_entry *fe_ce;
zend_string *fe_class_name = get_class_from_type(&fe_ce, fe_scope, *single_type);
zend_string *fe_class_name = get_class_from_type(fe_scope, *single_type);
if (!fe_class_name) {
continue;
}

inheritance_status status = zend_is_class_subtype_of_type(
fe_scope, fe_class_name, fe_ce, proto_scope, proto_type);
fe_scope, fe_class_name, proto_scope, proto_type);
if (status == early_exit_status) {
return status;
}
Expand Down
1 change: 1 addition & 0 deletions Zend/zend_map_ptr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,5 +103,6 @@
ZEND_API void zend_map_ptr_reset(void);
ZEND_API void *zend_map_ptr_new(void);
ZEND_API void zend_map_ptr_extend(size_t last);
ZEND_API void zend_alloc_ce_cache(zend_string *type_name);

#endif /* ZEND_MAP_PTR_H */
Loading