Skip to content

Implemented protobuf extension#7964

Open
huzhiguang wants to merge 2 commits into
facebook:masterfrom
baidu-lamp:baidu-lamp/protobuf
Open

Implemented protobuf extension#7964
huzhiguang wants to merge 2 commits into
facebook:masterfrom
baidu-lamp:baidu-lamp/protobuf

Conversation

@huzhiguang

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@fredemmott @mxw Can you help review this extension?

@fredemmott fredemmott assigned mxw and unassigned fredemmott and mxw Aug 25, 2017
@fredemmott

fredemmott commented Aug 25, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

also cc @binliu19

@RyanGordon

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

huzhiguang commented Aug 30, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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