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

[PHP] Optimized away hex2bin() call in generated code #8006

Merged
merged 6 commits into from
Nov 5, 2020

Conversation

haberman
Copy link
Member

@haberman haberman commented Nov 5, 2020

Generated code now embeds the binary descriptor directly instead of embedding hex and calling hex2bin().

This saves a small amount of CPU when importing PHP types.

@haberman haberman merged commit ea513d7 into protocolbuffers:master Nov 5, 2020
@haberman haberman deleted the php-internaladd branch December 6, 2020 20:27
copybara-service bot pushed a commit that referenced this pull request Jan 24, 2024
Follow up on #10404, the initial description:

> For the sake of performance, the hex2bin call was removed from the generated PHP code in #8006
However, after this PR all autogenerated files contain binary data, which makes it hard or even impossible to see using common tools (git and github included (i.e. [this file](https://github.com/protocolbuffers/protobuf/blob/main/php/src/GPBMetadata/Google/Protobuf/Struct.php)), as well as some code editors) since most software considers such files as binary ones and not as code.

Alexander suggested using a hex representation of string literals and I updated the original patch a bit, not hex-encoding printable characters.

Benchmarks from [#10404#issuecomment-1635939062](#10404 (comment))
```
The percent of printable chars: 0.83869 (80112 vs 15408)
hex parsing: 3.22371, length=382090
hex2Bin parsing: 1.18489, length=191059
hexAscii parsing: 0.89524, length=142306 (suggested option)
binary parsing: 0.26437, length=95542
```

Closes #13911

COPYBARA_INTEGRATE_REVIEW=#13911 from mikhainin:do_not_store_binary_data_inside_php_files 490958e
PiperOrigin-RevId: 601123255
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.

3 participants