Skip to content
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

Persistent Descriptor Pool #6899

Merged
merged 16 commits into from Nov 18, 2019
Merged

Conversation

TeBoring
Copy link
Contributor

@TeBoring TeBoring commented Nov 16, 2019

Previously, because it's possible that different php requests may have conflicting proto message (different message with the same fully qualified name), it is required that the internal descriptor pool is refreshed after each request, which hurts performance.

This change implements persistent descriptor pool. Users needs to make sure there is no conflicting messages by themselves.

By default, this feature is disabled. To enable it, add protobuf.keep_descriptor_pool_after_request=1 into php.ini. Or on command line, add -d protobuf.keep_descriptor_pool_after_request=1.

@TeBoring TeBoring changed the title Persistent Descriptor Pool Persistent Descriptor Pool [WIP] Nov 16, 2019
By default, it's off. Add protobuf.keep_descriptor_pool_after_request=1 to php.ini to enable it
@TeBoring TeBoring changed the title Persistent Descriptor Pool [WIP] Persistent Descriptor Pool Nov 18, 2019

DescriptorInternal* get_msgdef_desc(const upb_msgdef* m) {
upb_value v;
#ifndef NDEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems new, what is this? What does this NDEBUG do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NDEBUG is a standard macro during compiling, which means no debug mode.
This is somehow exposed by upb. We don't actually depend on it. Just to let upb happy.

ALLOC_HASHTABLE(proto_to_php_obj_map);
zend_hash_init(proto_to_php_obj_map, 16, NULL, HASHTABLE_VALUE_DTOR, 0);
static initialize_persistent_descriptor_pool(TSRMLS_D) {
upb_inttable_init(&upb_def_to_desc_map_persistent, UPB_CTYPE_PTR);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does upb_inttable_init do? This seems central to the theme about creating the persistent pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It initializes a custom hashtable implemented by upb.

internal_generated_pool_php = NULL;

if (!PROTOBUF_G(keep_descriptor_pool_after_request)) {
initialize_persistent_descriptor_pool(TSRMLS_C);
Copy link
Contributor

Choose a reason for hiding this comment

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

So just to confirm - the logic here is that: if the new php.ini config entry is not set (old behavior), we do this initialize_persistent_descriptor_pool (which I suppose is just current behavior), for every PHP request? (hence this RINIT function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@TeBoring TeBoring merged commit 3cae867 into protocolbuffers:master Nov 18, 2019
rafi-kamal pushed a commit that referenced this pull request Nov 19, 2019
* Make reserve names map persistent

* Add DescriptorInternal to map

* Use get_msgdef_desc in encode_decode.c

* Add persistent map for ce=>def and enum=>def

* Replace get_ce_obj

* Remove get_proto_obj

* Remove obsolete fields from Descriptor and EnumDescriptor

* Add cache for descriptor php values

* Add cache for descriptors

* Fix bug

* Avoid add generated file again if it has been added

* Fix the bug upb depends on null-ended str for look up.

* Initialize generated pool impl

* Turn down old generated pool

* Add init entry flag protobuf.keep_descriptor_pool_after_request

By default, it's off. Add protobuf.keep_descriptor_pool_after_request=1 to php.ini to enable it

* Fix zts build
@TeBoring TeBoring deleted the php-optimization branch November 20, 2019 00:57
nlhien pushed a commit to nlhien/protobuf that referenced this pull request Feb 28, 2020
* Make reserve names map persistent

* Add DescriptorInternal to map

* Use get_msgdef_desc in encode_decode.c

* Add persistent map for ce=>def and enum=>def

* Replace get_ce_obj

* Remove get_proto_obj

* Remove obsolete fields from Descriptor and EnumDescriptor

* Add cache for descriptor php values

* Add cache for descriptors

* Fix bug

* Avoid add generated file again if it has been added

* Fix the bug upb depends on null-ended str for look up.

* Initialize generated pool impl

* Turn down old generated pool

* Add init entry flag protobuf.keep_descriptor_pool_after_request

By default, it's off. Add protobuf.keep_descriptor_pool_after_request=1 to php.ini to enable it

* Fix zts build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants