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 "float" serialization loses precision #8165

Closed
bshaffer opened this issue Dec 24, 2020 · 5 comments
Closed

PHP "float" serialization loses precision #8165

bshaffer opened this issue Dec 24, 2020 · 5 comments
Labels

Comments

@bshaffer
Copy link
Contributor

bshaffer commented Dec 24, 2020

In both the protobuf native PHP library and PHP C-extension, I am seeing a loss of precision with floats:

message TestMessage {
  float foo = 1;
}
$initialMessage = new TestMessage();
$initialMessage->setFoo(-8.6107481E7);
echo "Initial message: ";
var_dump($initialMessage->getFoo());

echo "Serialized message: ";
$serializedMessage = new TestMessage();
$serializedMessage->mergeFromString($initialMessage->serializeToString());
var_dump($serializedMessage->getFoo());

When the C-Extension is enabled, the loss of precision happens immediately. When the C-extension is not enabled, the loss of precision happens when the message is serialized and deserialized:

// Without protobuf C-Extension
Initial message: float(-86107481)
Serialized message: float(-86107480)

// With protobuf C-Extension
Initial message: float(-86107480)
Serialized message: float(-86107480)
@acozzette
Copy link
Member

I haven't done the math carefully but this sounds like it could be just caused by the fact that the protobuf float type is only 32 bits. As I understand, PHP always uses double-precision floats and that would explain why the plain PHP implementation doesn't lose precision by itself but only after a serialize and parse. The C extension presumably distinguishes between float and double and therefore converts immediately to the 32-bit float, resulting in the immediate precision loss.

@haberman
Copy link
Member

haberman commented Jan 5, 2021

Yes, Adam is correct. The bug here is with pure-PHP, which should truncate this to float precision in the setter.

@bshaffer
Copy link
Contributor Author

bshaffer commented Jan 6, 2021

@haberman So are we saying this is an issue in the Pure-PHP protobuf library, and the way to fix it is to update float setters to truncate to single precision?

@bshaffer
Copy link
Contributor Author

bshaffer commented Jan 6, 2021

I added a fix in #8187 that passes for my test suites

@haberman
Copy link
Member

This was fixed in #8187.

@haberman haberman added the php label Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants