php performance problem

[cl51287]

php version is 7.0.24, protobuf extension is v3.4.1(also in v3.5.1),
When i test the performance use ab (nginx + php-fpm), system cpu become very high,

demo.php
<?php
require 'User.php';
require 'GPBMetadata/User.php';
new User;

user.proto
syntax = "proto3";
message  User {
    repeated uint32 uids =1;
    uint32 size = 2;
}

ab code is

ab -n 10000 -c 1000 http://127.0.0.1/demo.php

I found, the problem is $pool->internalAddGeneratedFile(hex2bin("***")); in GPBMetadata/User.php, this code take up 1ms cpu time, and every request all run this line code, so that system cpu become 30%, and if i add many new ProtoClass(10), the system cpu become 85%

[TeBoring]

Currently, the descriptor pool is built per request. For the second time you call new ProtoClass in the same request, the internalAddGeneratedPool will do nothing.

[cl51287]

@TeBoring I use release 3.5.1, but this phenomenon still appears. This phenomenon don't occur when i delete internalAddGeneratedPool this line.

[cl51287]

@TeBoring i know, the internalAddGeneratedPool will do nothing at the second time in the same request, but next request, it will be built again, it will cause cpu become very high, when many request visit server. Can this built action cache in php-fpm process ? Because the proto does not change in the different requests.

[zvuki]

@TeBoring Can it be cached in, say, apcu?

[zackangelo]

@TeBoring what was the use case in mind when having the DescriptorPool be rebuilt every request? Is it for long-running PHP servers?

Rebuilding the DescriptorPool every request is having a massively detrimental effect on performance for us because we have a ton of proto classes. Does the C extension have to do the same thing? Would you be amenable to us adding some methods to DescriptorPool that allow us to cache it externally?

[TeBoring]

"Because the proto does not change in the different requests" Is this guaranteed? @cl51287
AFAIK, this is not true. Every request are free to execute any php files, which may contain conflicting protos.

[TeBoring]

@zackangelo Yes, for long-running PHP servers

[zackangelo]

@TeBoring we can guarantee in our case that the descriptors won't change. would you be opposed to us introducing a caching mechanism for the descriptor pool?

[cl51287]

@TeBoring yes, you are right, it may be change, but most of the time it does not change, as zackanglo and zvuki say , can it be cached in?

[TeBoring]

One option is add a compile flag to tell c extension to cache it. Protobuf user needs to specify it explicitly to make sure they know what they are doing.

…

On Thu, Mar 1, 2018 at 17:00 cl51287 ***@***.***> wrote: @TeBoring <https://github.com/teboring> yes, you are right, it may be change, but most of the time it does not change, as zackanglo and zvuki say , can it be cached in? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4227 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE9H5fJ2ZlXeqS-sAsNX2DTiaUOsGvLBks5taJm6gaJpZM4RsqLv> .

[cl51287]

@TeBoring Can you tell me which option can cache it , i can't find it in document

[zackangelo]

@cl51287 the option doesn't exist, Paul is suggesting that as something we might build to enable this behavior.

@TeBoring how would you feel about a compiler mode that generated code that wrote bytes to a CodedOutputStream directly instead of relying the descriptor to do it dynamically?

[TeBoring]

@zackangelo I don't think it's a good idea. We have tried to minimize php level code. Although your suggestion may improve the performance of pure php implementation, for cpp implementation, it will be terrible. With your proposal, most of the code will move from cpp to php.

[TeBoring]

I would suggest add a global method to tell protobuf runtime not to deactivate descriptor pool after request is finished.

[cl51287]

I found protobuf compile the descriptor pool to memory for cpp implementation, is there a method can cache this object use pure php implementation?

[TeBoring]

Not yet.

[cl51287]

Does this feature consider adding to protobuf? This compilation will take up many cpu, and there is no method to cache the result of compilation for php implementation.

[TeBoring]

Not in this release. But will add it later.

…

On Mon, Mar 26, 2018 at 6:37 PM cl51287 ***@***.***> wrote: Does this feature consider adding to protobuf? This compilation will take up many cpu, and there is no method to cache the result of compilation for php implementation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4227 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE9H5e-bWUAtzc0nXsHnUqAoy-vE2YJUks5tiZfZgaJpZM4RsqLv> .

[cl51287]

thank you very much

[cl51287]

@TeBoring Has this problem been solved?

[TeBoring]

Sorry, I haven't added it.

[zackangelo]

@TeBoring wondering if you've given this any more thought? this is still a problem that heavily affects us in production.

[TeBoring]

Sorry for late reply. I'll add it soon.

[cl51287]

Sorry for late reply. I'll add it soon.

Will the next version add this feature? @TeBoring

[zackangelo]

@TeBoring just wanted to ping and see if you've had a chance to look at this :)

[TeBoring]

Fixed in #6899
Anyone wants to try that?

[TeBoring]

To use it,

1. Use the protoc in the change to regenerate code
2. Add protobuf.keep_descriptor_pool_after_request=1 into php.ini

[cl51287]

@TeBoring Thank you very much, we are waiting for a long time, try it first.

[TeBoring]

Should have been fixed by #6899