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

Encoding: Longs default value of 0 is not being set on the wire #639

Closed
r-tock opened this Issue Jan 12, 2017 · 14 comments

Comments

Projects
None yet
2 participants
@r-tock
Contributor

r-tock commented Jan 12, 2017

protobuf.js version: master

Is this intended? If the field is set java proto2 library does not ignore defaults and sets it on the byte stream

@r-tock r-tock changed the title from Longs default value of 0 is not being set on the wire to Encoding: Longs default value of 0 is not being set on the wire Jan 12, 2017

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

In general anything set (either on the object or present in the json from which it is parsed) should be on the wire.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 12, 2017

I always assumed that such default values would ideally be excluded on the wire for efficiency, at least that's what I'd expect. Do you know if this is an official implementation detail or if it just happens that the Java lib does it this way?

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

I guess it is official implementation detail based on both C++ and Java libraries (~8 years of exp now). I don't know about the other languages.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

With proto3 I presume you can make the argument that default values should be excluded. But I have definitely been used to the behavior of value being set in the proto2 world.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 14, 2017

Could you help get a release out with a fix for this issue. This is the last remaining issue with migrating our code-base.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 14, 2017

Sorry, have been busy fixing jsPerf today. On top of that, a fix for this would break a lot of stuff, as the entire default values on prototypes model is involved. Just using hasOwnProperty to solve this would yield terrible performance afaik (need to revalidate that).

see - but this could be broken if defined checks are optimized away.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 16, 2017

I've now tested an implementation that uses Object.keys to determine which values are present. Unfortunately, that's 33% slower than what we have now (bench drops from 513k to 342k ops/s).

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 16, 2017

Now testing an implementation using hasOwnProperty. This does in fact seem to drop by just a bit with all the default value checks removed. Investigating.

dcodeIO added a commit that referenced this issue Jan 16, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 16, 2017

As a side-product, generated toObject implementations are super fast now using hasOwnPropery instead of Object.keys.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 17, 2017

Works great! Now I need a new release and I push our new converted code base to QA!

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 17, 2017

Released on npm. Feel free to reopen if there are any issues!

@dcodeIO dcodeIO closed this Jan 17, 2017

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 19, 2017

@dcodeIO I am seeing more related issues. nulled/undefined string fields are being sent as empty strings. across the wire

Version: 6.5.0

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 19, 2017

Reference: https://github.com/dcodeIO/protobuf.js/blob/master/src/encoder.js#L88

Might be the case for required fields, yeah. Should throw on .verify though. Can you provide an example?

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 19, 2017

These are not required fields, lets continue conversation here #652

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment