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

Add the hasBinary flag #2838

Closed
wants to merge 9 commits into from
Closed

Conversation

Andrews54757
Copy link

@Andrews54757 Andrews54757 commented Jan 26, 2017

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

When an object is emitted, the hasBin() function is used which iterates through the object checking for binary data. (https://github.com/socketio/has-binary)

New behaviour

The hasBinary flag allows for you to specify whether the object you emit has binary data or not, which skips the inefficient hasBin() function. You can now do

socket.hasBinary(true).emit("hello",someobjectwithbinary); // Contains binary data

socket.hasBinary(false).emit("helloagain",objectwithnobin); // No binary data

socket.emit("bye",anotherobject); // normal function

Other information (e.g. related issues)

I found out that I could make socket.io two times faster by implementing this. Since in many circumstances a developer would know when he/she will send binary data (or not), the performance can be drastically improved by providing this information. This is because without it, the HasBinary function will run, which itinerates through everything in a object to check for binary data. This is very inefficient with large objects. This new flag allows for developers to fix this problem.

@darrachequesne
Copy link
Member

Hi Andrew,

Thanks for the pull request! From what I understand (and what that comment suggests), the json flag defined here and here was initially meant to be used that way (the payload was JSON.stringifyed when the flag was present).

It does not seem to be used anymore, but it's a good use-case here, what do you think? How about something like:

type: !this.flags.json && hasBin(args) ? parser.BINARY_EVENT : parser.EVENT

(in both places)

@Andrews54757
Copy link
Author

@darrachequesne Hi, I dont think that would work, as one of the main features of the hasBinary flag is so you can set it to true or false. Both options would override the hasBin function. If I had a large object containing a binary, the JSON flag wouldn't work as it does have a binary, so the HasBin function would then run, which I am trying to avoid. Also, the naming is less confusing if you use hasBinary, not JSON.

Thanks

@darrachequesne
Copy link
Member

@Andrews54757 Ok thanks, I understand. A few remarks though:

  • would withBinary sound better, for chaining calls?
  • could you apply the change to the Namespace class too? (io.hasBinary(<bool>).emit)
  • would (this.flags.hasBinary !== undefined ? this.flags.hasBinary : hasBin(args)) ? parser.BINARY_EVENT : parser.EVENT, look clearer?
  • could you update the documentation please?

@Andrews54757
Copy link
Author

@darrachequesne

  • withBinary has more characters than hasBinary, I personally enjoy hasBinary better.
  • Done, I applied it to the namespace
  • Done
  • Done - Except that I am not good at writing documentations. Might need some review.

@Swinkid
Copy link

Swinkid commented Jan 27, 2017

Doc looks good to me - Anyone disagree? :)

@@ -545,6 +555,17 @@ io.on('connection', function(socket){
});
```

#### Flag: 'hasBinary'

Specifies whether there is binary data in the data to send. Increases preformance when specified. Can be `true` or `false`.
Copy link
Contributor

@billouboq billouboq Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preformance -> performance
data to send -> emited data ?

@@ -327,6 +329,14 @@ Sets a modifier for a subsequent event emission that the event data may be lost
io.volatile.emit('an event', { some: 'data' }); // the clients may or may not receive it
```

#### Flag: 'hasBinary'

Specifies whether there is binary data in the data to send. Increases preformance when specified. Can be `true` or `false`.
Copy link
Contributor

@billouboq billouboq Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preformance -> performance
data to send -> emited data ?

@darrachequesne
Copy link
Member

Thanks for the updates! I am still not convinced about the hasBinary name though, anyone else thinking it looks like it will return a boolean? Is it just me?

@Swinkid
Copy link

Swinkid commented Feb 2, 2017

@darrachequesne Two minds about it - think something like containsBinary would be better - but I then feel that is too long.

@Andrews54757
Copy link
Author

I personally like hasBinary better because it is short and concise.

@Andrews54757
Copy link
Author

Andrews54757 commented Feb 17, 2017

@darrachequesne How about I expand this further. So it would be method() instead. where types are specified, such as method("binary") or method("string"). Then, this could be used in the future when there are other available methods. (or parseMethod?)

@Andrews54757
Copy link
Author

However, I feel that it would be performance issue to check strings. Any ideas?

@darrachequesne
Copy link
Member

Closed in favor of #2829 and #2923.

@wagenaartje
Copy link

Shame this wasn't merged. This single function is what is slowing my server down to a whole other level as i'm sending large JSON's. The socket.io-parser that @darrachequesne implemented is vague, especially with no documentation (except for a few examples).

I simply replaced all the code inside hasBinary to return false and it made my server perform so much better.

@Andrews54757
Copy link
Author

@wagenaartje Yes, I agree with you. Thats why I stopped using socket.io and built my own from scratch.

@darrachequesne
Copy link
Member

Merged as #3185, sorry for the delay.

@darrachequesne darrachequesne added this to the 2.1.0 milestone Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants