PennyDreadfulMTG / Penny-Dreadful-Tools

A suite of tools for the Penny Dreadful MTGO community
https://pennydreadfulmagic.com
MIT License
40 stars 28 forks source link

Replace all TypedDicts with dataclasses? #11310

Open vorpal-buildbot opened 1 year ago

vorpal-buildbot commented 1 year ago

Reported on Discord by bakert#2193

bakert commented 1 year ago

And Containers too??!

bakert commented 1 year ago

@dataclass(slots=True) probably gets us to dictionary-like speed with all the typechecking and dot access we could want. Python 3.10 but we're on that, right?

bakert commented 1 year ago

The days of desperately wanting to replace Container with slotted classes is actually far behind us because there are very few (looking at you matchups/achievements) pages where we load vast numbers of things into memory any more. And where those do exist we'd like to do the refactoring work to use decktables, etc.

However the typechecking is much nicer so we should probably do it anyway.

bakert commented 3 months ago

We should be aware that dataclasses (even with slots) may actually be slower than Containers:

[dent pd] pipenv run python3 -m timeit -s "from dataclasses import dataclass" -s "@dataclass(slots=True)" -s "class A: var: int" "A(1)"          
5000000 loops, best of 5: 84.9 nsec per loop
[dent pd] pipenv run python3 -m timeit -s "from shared.container import Container; c =Container({'x': 1}); c.x"                                  
50000000 loops, best of 5: 4.05 nsec per loop

Not only that but there are still places where use a lot of Containers. Or at least one – getting search results prior to merging with rotation information at /api/rotation/cards. I am probably going to have to refactor that to return only names and not Card objects, though, for perf, unless I go with more radical options.

bakert commented 3 months ago

What we really need to do (and what I am doing for /api/rotation/cards) is just NOT have a zillion objects in memory at any stage. Everything should be a db query that can be sliced with LIMIT. In the case where we have to combine decksite with a cards search (totally uncacheable) we can use the new find.search_names to return a list of card names and use that as a WHERE clause on our LIMIT-sliced query.

Then we can use dataclasses for better IDE hints and checking etc. without worrying about 85 nanoseconds versus 4 nanoseconds because we're creating hundreds of thousands of things that we're not going to show anyone.

Honestly it would be pretty nice to put some kind of global counter in Container.init and have it flag if we ever create more than a thousand things in memory doing pretty much anything.