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

Introduce caching to circumvent issue with using UserAgentParser.parse #13

Closed
wants to merge 1 commit into from

Conversation

hexgnu
Copy link

@hexgnu hexgnu commented Nov 12, 2013

What

So it seems to be a pretty recurring issue based on previous PR's that UserAgentParser.parse can cause performance issues. I found this out the hard way by loading 30 million user agent strings and not realizing that was how it was defined.

This PR aims to help by introducing a cache that allows users to still rely on the same functionality but enjoy the much faster speed.

How to test

I threw a few cache specific tests in there but also made sure the tests themselves still worked.

@toolmantim
Copy link
Contributor

You don't think the two mentions in the Readme is sufficient?

image

and:

image

@hexgnu
Copy link
Author

hexgnu commented Nov 14, 2013

Yea I realized very quickly and moved over to loading a constant.

I just personally felt that UserAgentParser.parse always loading the yml from the file system was unneeded since you could have effectively the same API with better performance.

@toolmantim
Copy link
Contributor

Yeah, I understand. It's just implementing our own instance cache feels like a problem that shouldn't be solved in a gem that parses user agent strings. And I'm not sure of the side-effects of adding a cache.

Perhaps we could just ditch the class method?

@hexgnu
Copy link
Author

hexgnu commented Nov 14, 2013

I am 100% for ditching the class method.

@toolmantim
Copy link
Contributor

Cool, I've opened #14 to remove the class method on next major revision bump.

@toolmantim toolmantim closed this Jan 21, 2014
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.

None yet

2 participants