ParseUserAgent, ParseOS and ParseDevice are not using the cache

[Greenscreen23]

Hi,

I was using this library for a project recently, and was really surprised that ParseUserAgent was significantly slower than Parse. After digging a bit into the code, I think this is because ParseUserAgent is not using the internal cache, whereas Parse is.

As both methods are documented in the README, I was expecting both to have similar performance, with ParseUserAgent being a bit faster because it has to parse less. Currently, both methods seem to be recommended for regular use, but ParseUserAgent seems significantly worse than Parse.

In my opinion it might be helpful to document this behavior in the README or change it, because it is currently faster to use Parse(ua_string)['user_agent'] instead of ParseUserAgent.

[xmo-odoo]

Adding a warning to the readme sounds like a good idea, the current caching behaviour is really basic (similar to the old lru cache, the cache is just nuked when full), as well as quite small (20 entries by default), so it would need to be updated more fundamentally to use either a FIFO replacement (which is what re now does), or an LRU.

A good option might be to stop caching Parser and slap an lru_cache on the sub-functions, that would increase the cost of a cache hit from Parse but would avoid duplicating entries between Parse and the sub-functions.

It'd also allow e.g. Parse and ParseUserAgent to feed each other's cache, though I don't know how common a need it is to use both Parse and the sub-functions in the same codebase.

An interesting alternative would be to keep a bespoke cache (though probably with a better replacement policy than the current) and allow partially filling entries so e.g. ParseUserAgent would look for an entry in the global cache, check if the key user_agent and if not set it, but then Parse would look for the same entry, check if all keys are set, and only update the ones missing (if any).

It would make Parse a fair bit more complicated and increase the cost of a cache hit slightly as Parse would need to check for the presence of 3 keys, but it seems like it could behave nicely (and unlike lru_cache wouldn't require dropping 2.7; dropping 2.7 and 3.6 might be a good plan regardless but according to pypistats.org they still represent about 4 and 15% of usage respectively which... seems a lot)

[masklinn]

@Greenscreen23 could you explain how you were using things? Did you explicitly bump up the cache size when using the library? With how small the cache is and the rough replacement policy I wouldn't have expected "significantly" performance degradation.

In the meantime I've opened #104 to at least add a note to the readme.

[Greenscreen23]

@masklinn I stumbled across this when we had to parse a dataset in one of my courses at the univerity. We had to parse a logfile with ~300.000 user agents, most of them being identical (real world data set). Here are the times I measured with the differnt functions:

Using Parse(ua_string)['user_agent]:
real 0m4,845s
user 0m4,813s
sys 0m0,027s

Using ParseUserAgent(ua_string):
real 1m7,321s
user 1m7,150s
sys 0m0,076s

I also did not increase the cache size, I just imported the functions and used them.

Thanks for opening the pr :)

[masklinn]

@masklinn I stumbled across this when we had to parse a dataset in one of my courses at the univerity. We had to parse a logfile with ~300.000 user agents, most of them being identical (real world data set). Here are the times I measured with the differnt functions:

Using Parse(ua_string)['user_agent]: real 0m4,845s user 0m4,813s sys 0m0,027s

Using ParseUserAgent(ua_string): real 1m7,321s user 1m7,150s sys 0m0,076s

I also did not increase the cache size, I just imported the functions and used them.

Thanks for opening the pr :)

Cool, yeah that is significant indeed. Is the dataset public per chance? Is it some sort of log file collected over some period of time? Might be interesting both as a way to build up plain benchmarks, and as a way to investigate changes to caching on something of a real-world dataset.

[masklinn]

And thinking about it more, it does make a lot of sense if the logs are "raw" and each page hit from a given visitor is an entry, identical entries would tend to cluster significantly, meaning even the small cache and simplistic eviction strategy would be helpful. Even more so if you decided to just sort the UAs before processing them.

[Greenscreen23]

It's not public as far as I know and I don't think it will be published. It looks like it was collected on demo servers to include as many interesting user agents, but there are still quite a lot of duplicates and "real world traffic".

[masklinn]

It's not public as far as I know and I don't think it will be published. It looks like it was collected on demo servers to include as many interesting user agents, but there are still quite a lot of duplicates and "real world traffic".

Got it, thanks nonetheless. I guess I'll just add a note for now so unfortunate souls get the information in the meantime, and try to add caching before we cut 0.15, because it's a lot more significant than I would have initially expected. I'll look around for a test dataset to bench with, hopefully one of the sibling project has one somewhere.

[masklinn]

TIL github goes and find "fixes" stanzas in PR descriptions as well as commit messages? That's weird. Anyway as noted above this was not intended to be closed, I want to implement better caching.