Improve from Binary/Json/Partial performance by roughly 30% #582
Conversation
|
I would greatly appreciate a review of this PR. It's a fairly small change with a big speed improvement. Thank you for your time! 🙏 |
| const msg: UnknownMessage = type.messagePrototype | ||
| ? Object.create(type.messagePrototype) | ||
| : Object.defineProperty({}, MESSAGE_TYPE, {value: type}); |
There was a problem hiding this comment.
Can you add a comment here explaining that Object.create is used to avoid calling the expensive Object.defineProperty for every new instance?
We should also explain that the second code path is only there for BC, so we know that we can remove it with a major version release.
|
Released in v2.9.2. |
Includes the newly generated protobuf-ts models based on GetStream/protocol#347 The newest version of `protobuf-ts` includes an important performance optimization that we can benefit from in our SFU WS message decoding flow. More about it: timostamm/protobuf-ts#582
|
Hi, Our code base relies on plain objects. Due to the changes, the outcoming objects from the generated code are no longer plain. We would love to still keep up to date with the updates but right now we are forced to use the old version of the library (2.9.1). |
|
@jcready, we're using the prototype to avoid Object.defineProperty's perf penalty, while still keeping the property non-enumerable - correct? @Berd74, I assume the object isn't considered to be plain because of its prototype? You could write a function that strips a message (and all messages it contains in properties) of its prototype. Can you give this a try? |
|
@timostamm I believe the issue is similar to the one mentioned in #618 where some other library depends on (or will only operate on) objects which have all or some of the As I mentioned in the PR summary my original plan for this was to simply create a protobuf-ts/packages/runtime/spec/message-type.spec.ts Lines 86 to 100 in e757a4a The issue was that Jasmine's // Objects with different constructors are not equivalent, but `Object`s
// or `Array`s from different frames are.
const aCtor = a.constructor,
bCtor = b.constructor;
if (
aCtor !== bCtor &&
isFunction(aCtor) &&
isFunction(bCtor) &&
a instanceof aCtor &&
b instanceof bCtor &&
!(aCtor instanceof aCtor && bCtor instanceof bCtor)
) {
diffBuilder.recordMismatch(
constructorsAreDifferentFormatter.bind(null, this.pp)
);
return false;
}The biggest issue with switching to the class-based approach is that if that change already broke our tests then how likely is it that we still end up breaking someone else's tests that perhaps also used some form of Alternatively we use the odd solution in #618 which basically pulls in all the stuff from the The library mentioned in that PR was mobx which has checks like these in place: if (!isPlainObject(target) && !isPlainObject(Object.getPrototypeOf(target))) {
die(`'makeAutoObservable' can only be used for classes that don't have a superclass`)
}Where const plainObjectString = Object.toString()
export function isObject(value: any): value is Object {
return value !== null && typeof value === "object"
}
export function isPlainObject(value: any) {
if (!isObject(value)) {
return false
}
const proto = Object.getPrototypeOf(value)
if (proto == null) {
return true
}
const protoConstructor = Object.hasOwnProperty.call(proto, "constructor") && proto.constructor
return (
typeof protoConstructor === "function" && protoConstructor.toString() === plainObjectString
)
} |
This is our solution right now. After receiving objects from protobuf-ts (in our project: after using services like fetch requests, packets from webSocket, or packets from WebRTC data channels) we use a function that deeply checks if the object prototype has symbol before transform: The solution is not perfect but it works. We hope in the future we will be able to get rid of this transformation. @jcready I looked at the issue mentioned in #618 and I can confirm that we are having the same problem. We also use mobx. |
You're right. Thanks for digging up the details. I'm afraid that anything but a plain object (only own properties, no cyclic references, only JSON serializable types) will cause an issue in one framework or the other. At the same time, Protobuf messages ideally have the schema information attached to the object, so all necessary information for serialization and reflection is in one place. It can be a bit frustrating that it‘s extremely rare to see a framework provide hooks so that behavior can be adjusted to the domain models. #618 isn't going to solve this issue completely, but it does look like an improvement. |
As mentioned in #306 (comment) there is a non-trivial performance hit for using
Object.defineProperty(). Searching around you can find plenty of other mentions of this.Originally my plan was to create a
MessageInstanceclass property on the baseMessageTypeclass and then callconst message = new this.MessageInstance()inside thecreate()method. Unfortunately jasmine seems consider two object to NOT be deeply equal to each other when one was created vianew this.MessageInstance()vs. an object literal with the exact same structure. However, if instead we utilizeObject.create()and pass in our prototype as the first argument (which should effectively be the same thing) then jasmine will consider the two objects deeply equal 🤷.This change adds an optional
messagePrototypeproperty to theIMessageTypeinterface (I'm hoping this isn't considered a backwards incompatible change). ThismessagePrototypeis created inside theMessageTypeconstructor and is stamped with the non-enumerableMESSAGE_TYPEsymbol pointing back tothis. Performing the stamping here means that we only are penalized for theObject.defineProperty()once per message "class" instead of once per message "instance".I believe this change should be backwards compatible. Any old generated code should continue to work with the new runtime version, but newly generated code (only when optimized for speed) will not work with old runtime versions (optimized for size code will continue to work since it would go through
reflectCreate()instead of generated code).The below benchmarks were run with nodejs v20.6.1 (latest as of time of writing). According to these protobuf-ts (speed, bigint) will now read binary messages ~20% faster than protobufjs.
Before:
After