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

"large performance hit" #68

Closed
snoyberg opened this issue May 30, 2014 · 7 comments
Closed

"large performance hit" #68

snoyberg opened this issue May 30, 2014 · 7 comments

Comments

@snoyberg
Copy link
Contributor

The quotes are to indicate that I'm passing on a user report; more information is available in the Yesod mailing list thread. This appears to be a serious performance issue in either websockets or io-streams. Unfortunately, I'm not familiar enough with either codebase to really localize this further. I ended up writing a mini websockets implementation which Geoff compared against websockets which, while still slower than normal GET requests, did not include the major performance hit mentioned at the beginning of the thread.

Pinging @gregorycollins in case you want to look into the io-streams aspect. I have absolutely zero information on whether this is a problem in io-streams, or simply with how websockets calls io-streams.

@jaspervdj
Copy link
Owner

Hmm, not sure what could have caused this, but the websockets library is in need of a couple of benchmark-optimize iterations anyway. I will take a stab at that this month.

@jaspervdj
Copy link
Owner

@snoyberg Just a heads up, it turns out that it is actually very easy to remove the io-streams dependency entirely. I hope to release this work soon-ish.

@snoyberg
Copy link
Contributor Author

@jaspervdj That's good news. Does io-streams appear to have been the culprit here?

Out of curiosity, are you also considering removing attoparsec, or is that more deeply ingrained? I only ask because, when I did my proof-of-concept implementation, I didn't find the need to use a parsing library, since the protocol is fairly simple.

@jaspervdj
Copy link
Owner

Looks like it was just some unoptimized code on my side and some overhead of converting between the streaming concepts.

@jaspervdj
Copy link
Owner

@snoyberg So I don't really need Attoparsec for parsing packets (in fact, it would probably be faster without Attoparsec so I'll drop it there), but this package also has some HTTP request parsing (for the built-in server) and Attoparsec comes in really useful there.

@snoyberg
Copy link
Contributor Author

What about splitting out the core websockets code and the HTTP code? It's not really vital, but it would reduce the scope of what is included in the package, and hopefully thereby make it easier to work with the codebase/discover future memory leaks.

@domenkozar
Copy link
Collaborator

domenkozar commented Dec 29, 2023

A lot of the code has changed since 2014, I've opened #235 to get us a better idea about performance.

I'm going to close this for now as I don't see anything actionable or reproducible.

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

No branches or pull requests

3 participants