-
Notifications
You must be signed in to change notification settings - Fork 107
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
Implement maps #82
Implement maps #82
Conversation
Generated code for 'use strict'; // code generated by pbf v3.1.0
// Envelope ========================================
var Envelope = exports.Envelope = {};
Envelope.read = function (pbf, end) {
return pbf.readFields(Envelope._readField, {kv: {}, kn: {}}, end);
};
Envelope._readField = function (tag, obj, pbf) {
if (tag === 1) { var entry = Envelope._FieldEntry1.read(pbf, pbf.readVarint() + pbf.pos); obj.kv[entry.key] = entry.value; }
else if (tag === 2) { entry = Envelope._FieldEntry2.read(pbf, pbf.readVarint() + pbf.pos); obj.kn[entry.key] = entry.value; }
};
Envelope.write = function (obj, pbf) {
if (obj.kv) for (var i in obj.kv) if (Object.prototype.hasOwnProperty.call(obj.kv, i)) pbf.writeMessage(1, Envelope._FieldEntry1.write, { key: i, value: obj.kv[i] });
if (obj.kn) for (i in obj.kn) if (Object.prototype.hasOwnProperty.call(obj.kn, i)) pbf.writeMessage(2, Envelope._FieldEntry2.write, { key: i, value: obj.kn[i] });
};
// Envelope._FieldEntry1 ========================================
Envelope._FieldEntry1 = {};
Envelope._FieldEntry1.read = function (pbf, end) {
return pbf.readFields(Envelope._FieldEntry1._readField, {key: "", value: ""}, end);
};
Envelope._FieldEntry1._readField = function (tag, obj, pbf) {
if (tag === 1) { obj.key = pbf.readString(); }
else if (tag === 2) { obj.value = pbf.readString(); }
};
Envelope._FieldEntry1.write = function (obj, pbf) {
if (obj.key) pbf.writeStringField(1, obj.key);
if (obj.value) pbf.writeStringField(2, obj.value);
};
// Envelope._FieldEntry2 ========================================
Envelope._FieldEntry2 = {};
Envelope._FieldEntry2.read = function (pbf, end) {
return pbf.readFields(Envelope._FieldEntry2._readField, {key: "", value: 0}, end);
};
Envelope._FieldEntry2._readField = function (tag, obj, pbf) {
if (tag === 1) { obj.key = pbf.readString(); }
else if (tag === 2) { obj.value = pbf.readVarint(true); }
};
Envelope._FieldEntry2.write = function (obj, pbf) {
if (obj.key) pbf.writeStringField(1, obj.key);
if (obj.value) pbf.writeVarintField(2, obj.value);
}; |
@kjvalencik We're looking to migrate all our apps to proto 3 and one of the nice features is maps! Have you been running this successfully in production? |
@edbo I have not run this branch in production. However, I have been running this library in production for over a year. |
Yeah we're having great success with PBF in general but this map thing is becoming an issue for us. In your opinion is this pull request ready for prime time or are there more steps required to get this merged? We'd be willing to help out a bit if there's still more to do. I haven't dug deeper yet but I can't really tell if your tests actually encodes and decodes maps. I'm also wondering if there are any performance implications. You mention it's sub optimal but I'm assuming this is only the case for where maps are present. We actually use pbf in the browser (which means performance is huge problem) so we're also thinking along the lines of potentially performing some bench marks on changes to see how it changes over time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for missing this PR! It looks pretty good to me.
@edbo It's feature complete, but it hasn't been reviewed. It's a guess that it's not optimal, but I haven't actually benchmarked. It depends on usage, but my guess is that for small-medium sized maps it performs acceptably. It's not idea, because it creates Objects for key / value, pairs, just to aggregate that data into another map. That could be streamlined. |
var name = 'obj.' + field.name; | ||
|
||
return 'for (' + (numRepeated ? '' : 'var ') + | ||
'i in ' + name + ') if (Object.prototype.hasOwnProperty.call(' + name + ', i)) ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to do this guard, or simply assume those are simple object literals? And why would an object not have a hasOwnProperty
? To support Object.create(null)
cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is the write code, you don't have control over the objects sent to it. In my opinion, it's better to default to safe code and this could be relaxed where the speed is necessary.
Yes, Object.create(null)
won't have a prototype chain and therefore no hasOwnProperty
. I don't feel strongly about that, it's a technique I picked up from the Airbnb style guide.
@edbo if your application is especially performance critical you may want to avoid maps. Even with a better implementation, it would require transitioning the hidden class repeatedly and would be sensitive to insertion order. If you have a known set of keys, messages should perform better. I'll add some benchmarks to see how this stacks up against plain associative arrays.
It looks like encode times are nearly identical. Decoding is slower, but my less than a factor of 2. No changes in benchmarks for things unrelated to map. |
Nice that's awesome, this pretty much solves most of the issues we're having now! Massive thanks @kjvalencik |
Implements #81
@mourner Pushed a small change to only add |
Implements #81