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

Implemented protobuf extension #7964

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

huzhiguang
Copy link
Contributor

Summary:
Implemented protobuf extension.

php extension git address:https://github.com/allegro/php-protobuf

Generate php protobuf files you need to run using HHVM,e.g:

hhvm protoc-gen-php.php foo.proto

@huzhiguang
Copy link
Contributor Author

@fredemmott @mxw Can you help review this extension?

@fredemmott fredemmott assigned mxw and unassigned fredemmott and mxw Aug 25, 2017
@fredemmott
Copy link
Contributor

fredemmott commented Aug 25, 2017

I'm not going to be able to look at this really. That said:

  • the reader/writer stuff doesn't belong in the HPHP namespace
  • the f_printf thing is very unclear
  • zend tests are only for tests copied directly from PHP itself
  • there's an alternative implementation over at https://github.com/quizlet/protobuf/tree/hhvm - we should be comparing to that, not reviewing in isolation - cc @RyanGordon

@fredemmott
Copy link
Contributor

also cc @binliu19

@RyanGordon
Copy link
Contributor

Just FYI, I actually haven't been able to make any progress on that and because the pure-PHP Protobuf library works on HHVM (I've been maintaining it for HHVM compat and merging patches in the latest protobuf master as needed) so we haven't prioritized work on it. The JIT optimizes the pure-PHP Protobuf library pretty well but I'm sure this C implementation would still be several ms faster.

Since this is the furthest implementation of protobuf for HHVM, I would be happy to help test it and what not @huzhiguang

Also, are there any preferences about contributing this to the google/protobuf repository versus baked directly in HHVM? It would be nice if it was just built in directly but if it was merged in to the google repo it would probably get more maintenance. Thoughts?

@RyanGordon
Copy link
Contributor

Also, I just looked through and this seems to only implement a smalls subset of the Protobuf extension. Is that correct?

@fredemmott
Copy link
Contributor

Are there significant advantages to having an extension for this instead of using the PHP library? In many cases, C++ extensions end up being slower than JITed PHP.

@RyanGordon
Copy link
Contributor

Ah interesting; The main performance problem we see in the PHP Protobuf library (without JIT) is that for every single request these functions listed here are called over and over again and are very expensive: https://github.com/google/protobuf/search?l=PHP&q=initOnce%28&type=&utf8=%E2%9C%93

With the JIT on these become significantly faster but there is still an impact. I think the Protobuf PHP C extension actually only initializes those things once for the duration of the PHP process daemon rather than for every request

@huzhiguang
Copy link
Contributor Author

@fredemmott

  • the reader/writer stuff doesn't belong in the HPHP namespace

    • re: I will add HPHP namespace to reader/writer
  • the f_printf thing is very unclear

    • re:I will optimize print mode instend of f_printf
  • zend tests are only for tests copied directly from PHP itself

    • re:I will add more test code
  • there's an alternative implementation over at https://github.com/quizlet/protobuf/tree/hhvm - we should be comparing to that, not reviewing in isolation - cc @RyanGordon

  • Protobuf avg data:

Engine+extension unpack(avg) pack(avg) total
hhvm 3.0 + phplib 0.9ms 3.6ms 4.5ms
php 5.4 + phplib 5.4ms 2.8ms 8.2ms
php 5.2 + so 0.27ms 0.07ms 0.34ms
  • Protobuf max and mix data:
Engine+extension Min unpack Max unpack Min pack Max pack
hhvm 3.0 + phplib 0.14ms 15.7ms 0.26ms 6.6ms
php 5.4 + phplib 0.9ms 23ms 0.3ms 4.5ms
php 5.2 + so 0.084ms 0.45ms 0.0097ms 0.14ms

Test data proves that C extension performance is better than php jited extension

@fredemmott
Copy link
Contributor

Leaving the bulk for others (and I'll follow-up to make sure that happens).

Performance comparisons against a more recent version would likely be useful,

zend tests are only for tests copied directly from PHP itself

re:I will add more test code

Remove from zend/, put in slow/

@huzhiguang
Copy link
Contributor Author

huzhiguang commented Aug 30, 2017

Performance comparisons against a more recent version would likely be useful,

I will follow this comparison test and give the data.

Remove from zend/, put in slow/

I will modify the code and PR it to you

@facebook-github-bot
Copy link
Contributor

@binliu19 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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.

6 participants