-
Notifications
You must be signed in to change notification settings - Fork 3.6k
changing boolean to an empty struct #656
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
Conversation
ferhatelmas
left a comment
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.
LGTM
|
This a good change because The extra memory for the |
|
I had made a similar change before #566 |
On the contrary, the point of this example is to use as a base for your own application. A user could presumably use this code to build an enterprise WebSocket application with tens of thousands of users. In this hypothetical, the extra memory would not be negligible. |
|
With a savings of single digit number of bytes per connection, that 10K enterprise server can now serve 10,003 connections. Because the hub part of the example assumes that connections are terminated in a single process, that part of the example is not a good starting point for any kind of serious application. |
|
Might be nice to document that then in the example. |
|
Mixed opinions here... The memory being negligible and this being an example makes me think this isn't really necessary but I also see no reason not to make this change as well as it is technically an improvement. Otherwise, if someone is building an enterprise application based on library examples IMHO they deserve any misbehavior they encounter by not using some sense in their application design. In my experience, library examples are always contrived and/or minimal to convey a single point and should never be used as a basis for an enterprise application (I thought this was common sense, I've never seen anyone take the time to write in an example that it's just an example and not to use it as a basis for an enterprise application; it's implied by the fact that it's an example). That said if you really want to improve it I'm sure no-one here would argue with the addition of code comments explaining why something is potentially unsafe. |
|
The code specific to websockets is in client.go. The code in client.go is an OK starting point for an enterprise-grade application. A disclaimer about suitably of the example might throw developers off from using this code. I'd think it's obvious that the 50 or so lines of code in hub.go is not a starting point for an enterprise-grade messaging service. |
|
I'm closing this one out as I think the discussion has run its course. Although examples should try to avoid introducing bad habits, the actual impact of bool vs struct{} here is tiny. Most developers run into FD limits and network issues well ahead of memory limits (which are also easy to profile). |
A tiny change: in the example hub, we should use a map of empty structs instead of a map of bools.
Using booleans is a bug, since it wastes memory for no reason.