-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Move default values to prototypes and ditch nulls #380
Conversation
I'm not sure I like this change because it will break Hidden Class optimizations since the object will change itself at runtime. This will also destroy the Inline Caches for methods that take in these objects as parameters. The object should not change the properties it has at runtime, at construction it should have all the properties it would ever have. |
@englercj Perhaps you can suggest a test script that we could use to measure the performance difference? Unless it's significant, I would argue that compliance to the spec and API simplicity is more important. |
@seishun The performance difference is heavily different because one way will generate optimized (lithuim generated) code, the other will be full compiled (unoptimized code). Depending on your application this can be unimportant or application breaking. Measuring the performance difference in raw numbers of some test script doesn't mean much except in the context of your application, but breaking well known engine optimizations and forcing these objects to be hidden class polymorphic (and therefore break all functions that use them as well) is not a good idea. I would move to a different library for our applications if the parsing of messages caused it to return a different hidden class every call. We process too many messages at volume to suffer a deoptimization of that nature. If you are interested in the difference between calling a monomorphic, polymorphic, or megamorphic functions (these changes would make using functions become megamorphic eventually) then I suggest this article and this microbenchmark. All of that said, I'm not suggesting we don't follow the spec. I'm suggesting that changing the properties of the object by adding or removing them at runtime is not the correct solution. |
It means more than speculation.
Surely you wouldn't move without seeing the numbers first? In that case, it would be beneficial for everyone involved if you could test this solution now.
I'll be glad to see other suggestions if I see that this solution is unacceptable performance-wise. Also, if you insist on following optimizations guidelines, Google seems to disagree with your statement that "at construction [the object] should have all the properties it would ever have": https://developers.google.com/speed/articles/optimizing-javascript#initializing-instance-variables ...Unless you didn't mean own properties, in which case I don't see how it applies here. |
I actually like the |
I'm not speculating, I'm trying to explain to you how V8 works. I even included a benchmark and an article that explains the compilation process I am describing.
Do the numbers I linked not count? Having benchmarked our application and knowing I don't want it to run at half speed not good enough? We work pretty hard to hit optimized paths in the compilers due to the scale of the application. Suddenly removing those optimizations is not acceptable.
Huh? By placing value properties on the prototype, you essentially let V8 do the initialization of those properties for you on Hidden classes are per-object (which includes
Pretty much, the properties that the message will hold need to be added at construction time then not deleted. Each delete causes a hidden class transition which is expensive, and having the multitude of hidden classes (depending on the diversity of how optional a messages arguments are) can cause megamorphic use of it. This can be unacceptable for many use cases, including ours. |
In that case, I don't understand your gripe with this solution. There is always going to be a property on the object for each field, either own or inherited. |
https://github.com/dcodeIO/protobuf.js/pull/380/files#diff-3046de64cab49373b52464f636b51688R132 No, there isn't. You explicitly have delete, which removes the property. That transitions the object to a new Hidden Class. The object, and the Being on the prototype is fine, being on the object is fine, changing either after construction is a no-no. |
Removing a property doesn't remove it from the prototype. > var o = Object.create({foo: 5})
undefined
> o.foo = 'bar'
'bar'
> delete o.foo
true
> o.foo
5 |
Never said it did, or if I did I mispoke. I said removing the property from the object changes the Hidden Class. I said that the object I recommend you read the articles I posted, because I don't think you understand what I am saying. This is another good one as well: https://www.smashingmagazine.com/2012/11/writing-fast-memory-efficient-javascript/ And of course I've only be speaking in terms of hidden classes, I haven't even mentioned the can of worms that can come by the object moving into Hash Table mode instead of being Class-backed. That would suck even worse. |
Now you're contradicting yourself. So does it or does it not matter where the property is? Assuming your current stance is that it does:
Then yes, it does disagree with you. If a property is on the prototype, then it doesn't magically become the object's own property after construction – it remains inherited. The two snippets are semantically different. |
Well, you have (at least) two (more with inheritance taken into account) hidden classes here: One for the single prototype that contains the default values as its "own" properties (the one-time generated The prototype (generated class) is not the issue here, the issue are the instances of them (those where delete is used on properties and which share a common hidden class ideally). It's not about changing the prototype, it's about changing objects that share their exact property layout. As I said, I really like the |
@englercj Not very classy to edit your comments to make them look more convincing after they've been responded to.
Are you implying that the snippet from Google assumes the properties
Have you read them? Because it seems you have skipped over or are intentionally ignoring the second-to-last paragraph in the first article you linked:
|
Forgot that this is a dick-measuring contest, my bad. Ignore my comment. |
I feel sorry but I should join your 'dick-measuring contest' because that was my initiative to push this feature implementation Dear all ProtoBuf.js maintainers, let's read this short article from ProtoBuf (and Cap'n'Proto) dev team: https://developers.google.com/protocol-buffers/docs/overview#a-bit-of-history << New fields could be easily introduced, and intermediate servers that didn't need to inspect the data could simply parse it and pass through the data without needing to know about all the fields.>> And what you just did? You implemented something like tricky V8 - based binary stream reader and writer that work excluding data scheme. THAT'S IT. The whole feature set mentioned above were completely IGNORED the whole your implementation story, so please do not even try to tell that this is ProtoBuf implementation. This is ProtoBuf API - compatible V8 - based reader and writer implementation. That's all. You have no possibility in code to get list of fields that were changed! What's the point to use your implementation? Your current version just save the traffic bytes, ok, well done guys, well done! I'm just wondering and can't imagine your .js code, how do you work with rich protocols (many message with many fields)? Looks like you do tens of if / else or switch / case statements in your code. In our case this is not tens, but hundreds of this useless code jumps because of your 'implementation'. Now, If you have some little doubts about how your super - duper - faster - hidden V8 - based code works, just ask the ProtoBuf initial developer what does he think about implementation with empty reflection feature set and your '.js null - features'. |
@seishun I edit my posts when I have a typo, realize I said something wrong, or feel that I need to clarify. I reread my post and edit it until I'm satisfied with it. I'm not trying to "win" anything because this isn't an argument, I'm trying to educate you. I think the disconnect is that you are interpreting "Hidden Class" as the prototype object, but those aren't the same thing. Hidden Class !== Prototype.
Changing the value isn't the issue (unless you are changing the type), adding and removing properties that change the structure of the object is the issue. I've mentioned multiple times now that I have both profiled, and read the IR of our application. And I'm here to tell you we cannot suffer an deoptimization of this scale. For most applications or websites this probably doesn't matter. For high-performance node.js application processing millions of messages per second (like ours) it matters, a lot.
They are different, but neither of them change the structure of the object after construction. The optimization mentioned there is a different one than what we have been discussing. @dcodeIO I'm done now, I don't understand why this is spiraling out of control. I'm trying to be reasonable and explain how this works, but I just keep getting met with "nuh-uh" and ad hominem attack. I honestly didn't think this would go like this, but you're right that the discussion is lost here and has degraded into a dick measuring contest. Sorry about that. Do what you feel is best, you have been honoring rules like these while building this lib for a bit now so I trust you. In fact, its good to see that you actually do understand engine mechanics:
Thank you for understanding, and we look forward to working on future library optimizations. |
Does anybody of repository users can provide us the complete example of how they use optional / repeated fields on .js - side? I mean, what's the point to discuss the performance of wrong code implementation? I completely do not understand the reasons why people use this. Just for compatibility with non - .js server side data exchange components? To reduce traffic only? If so, you can write your own strict binary reader / writer that exclude the data scheme in traffic, do not marshalling each time while you parse your message and that's it. But it will be not the ProtoBuf implementation, sorry, guys. The ProtoBuf technology means the complete feature set and here we have a lot of features missed, especially in reflection infrastructure. The initial proposal and implementation mentioned in the top of this discussion is just the one attempt to implement this feature set in order to provide complete ProtoBuf technology implementation. This attempt faced with a lot of 'it works perfectly now for us, why we should add your code' (sick). One more question, does anybody of repository maintainers used other ProtoBuf implementations? C++ / Java - based? If someone can positively answer this question, I see no more pros for push this code further, in production... |
I understand how hidden classes work. You haven't told me anything new. You're missing my point that speculating about performance based on one known optimization is unreasonable, as the very article you linked clearly points out.
You didn't answer my question clearly, so I assume the answer is "No". As you hopefully know, assigning to either
Protobuf.js does change the type currently. Between |
@seishun the page you are linking to is really-really old, I think it even predates V8 itself Please don't use it to justify any sort of optimizations. Advice given in the section "Initializing instance variables" might have been a very clear cut back in the day, but these days it comes with a very complicated trade-off - and usually it's impossible to predict whether it will improve the performance and memory consumption of your code. The opposite is also true - it's impossible to predict whether it will regress performance either. Requires benchmarking and detailed analysis. It is true however that if the field is almost never present then you might at least save some memory (potentially sacrificing some performance due to polymorphism). [Note: I did not look at the code in question, I am just commenting on the advice from "Initializing instance variables" in general] |
That's exactly what I've been saying the whole time.
I'm only using it to show that blindly following optimization advice can lead to contradiction. |
@dcodeIO It's been a month. Since the only person who was against this change due to performance concerns still hasn't provided benchmarks supporting their claims, can we merge this or at least move on to discussing other aspects besides performance? |
Hello. I really need this code. I am using proto3 and I am having several issues with the defaults. Could someone please review it and merge? |
+1 to what @michelgb said |
I don't think performance matters if we're not spec compliant. I'd like this PR to be merged. Alternatively update the readme so people that need proto3 support are aware of the issue. |
@bootstraponline agreed, any progress on this? also @seishun hey from node-steam |
I think we're just waiting for someone with write access to press the merge button. |
@dcodeIO |
Oh, this was obviously closed automatically while moving branches. |
This fixes #200 by implementing the API described in #200 (comment).
optional bool foo = 0;
is now equivalent tooptional bool foo = 0 [default = false];
.ProtoBuf.populateDefaults
option is removed.The changes are breaking for some (unlikely) use cases, but I tried to be as conservative as possible. Also note that all the above only applies to
proto2
. The behavior ofproto3
should be exactly the same as before.Also fixes #319, #373.