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

Add slots for tag class #22

Merged
merged 1 commit into from
Sep 6, 2021
Merged

Conversation

Jordan-Cottle
Copy link
Contributor

@Jordan-Cottle Jordan-Cottle commented Aug 21, 2021

It looks like issue #21 mentions that you wanted to use __slots__ in this project. I'm not familiar with the structure of the project or where all the classes are defined, so this PR only adds __slots__ for the one class that had the todo comment for it.

All of the rest of the classes should be just as simple. You just have to list out all of the expected instance attributes for each class. If a class is simple and just assigns all of it's attributes normally with self.<name> = ... in it's __init__ or elsewhere, then you can just search for self.\w+ = to locate names easily.

Using __slots__ instead of the default __dict__ for attributes is pretty straightforward unless you have classes where instance attributes are dynamically set (not just appear to be set with a __getattr__ or __getattribute__ that intercepts attribute access).

I'm not sure this change warrants any extra tests since the existing test suite covers all of the attribute assignments in the tag class

$ coverage run -m unittest discover tests
<lots of output, enough lines with just hi to fill up my entire terminal history>
...
----------------------------------------------------------------------
Ran 259 tests in 19.748s

OK
$ coverage report -m | grep html
Name                                                                                Stmts   Miss  Cover   Missing
-----------------------------------------------------------------------------------------------------------------
domonic/html.py                                                                       294     45    85%   65-66, 72-78, 103-104, 121-122, 130-131, 155-158, 169-172, 178-181, 187-190, 194-196, 225-227, 265, 272, 322, 487, 490, 493, 512, 515
tests/test_html.py                                                                     77     25    68%   47-56, 60-91, 95-133, 137-358, 362-532, 537-805, 845

@Jordan-Cottle Jordan-Cottle mentioned this pull request Aug 21, 2021
@byteface
Copy link
Owner

This looks really exciting!... I'd read about slots probably for 2-3 hours but the confidently using them part I hadn't got to. I didn't want to put them in blindly without fully understanding the repurcussions. I kinda sussed where they might go in the file and was getting close to your solution. but hadn't took the next step yet. I'll try to pull this down and run some of my examples through it etc. I'm sure it's fine and your explanation is really helpful in me understanding them.

@Jordan-Cottle
Copy link
Contributor Author

Jordan-Cottle commented Aug 21, 2021

I'm glad I was able to help!

Using __slots__ essentially is just a trade off of some of the dynamic power of python objects (which doesn't appear to be in use here, and should really only be used where there's a good reason to do so) for better performance.

By default, objects can have whatever attributes assigned to them. Which is why you can just plop self.<whatever> = <some value> anywhere in a method (like __init__ or any other function you define in a class without the @classmethod or @staticmethod). Whenever an attribute is created or accessed, it goes to the object's __dict__. So foo.bar would look up foo.__dict__["bar"] if foo was an object with attribute bar.

When you use __slots__ the class essentially automatically creates an @property with a setter for every name in the list you provide and prevents creating a __dict__ from being created (unless you add "__dict__" to the __slots__ list) so that no unexpected attributes can be defined on the fly like they could before.

Since dictionary objects are data structures that use up more space than just the data they hold, and accessing things through them has some overhead (but is generally an O(1) operation), using slots can save memory by not allocating memory for __dict__ and increase attribute lookup performance by skipping __dict__[attribute_name]lookups.

It's generally only considered/done for classes that have a ton of instances created or lots of attribute accesses that need to be fast.

@byteface
Copy link
Owner

that's a really helpful explanation and is really good and helping me get it. I guess my question with them was. if someone extended a tag. would their extended thing be able to have editable properties. as that would make it a fine i felt as most cases could be covered that way if a person really needed to extend a tag. but really they should just be returning them.

i'd like to also try and benchmark it on some of the bgger templates i have to see what the performance gains are. I didn manage to parse a page once into 5k nodes and was getting memory issues. so I had wondered if investigating this would help there. But really what i shoudl do is try set up some tests to show peformance gains. then consider where else i may be able to add them.

@Jordan-Cottle
Copy link
Contributor Author

Benchmarking things before attempting to optimize them is a pretty good idea. It's also nice to know how much of an impact a performance change makes.

For inheritance, the __slots__ vs __dict__ question doesn't really matter. Since setting __slots__ on tag only affects the names in the tag class. Any subclasses should be able to make independent decisions about any attribute that they add as well.

Here's a quick gist you can see that shows that subclasses that don't mention __slots__ will behave as expected.

@byteface
Copy link
Owner

amazing.! that's exactly the test and question I wanted to try. It looks like this should be good. I'll be adding this in at some point soon but will probably play a little first with the benchmarking thing as I've wanted to do a bit of that. thanks so much by the way. I'm feeling a lot better about slots

also thanks again for the timeout methods. they are huge wins. we had a huge number of clones yesterday so it must have been highly anticipated.

@Jordan-Cottle
Copy link
Contributor Author

You're welcome!

I'm glad I was able to help.

@byteface
Copy link
Owner

@Jordan-Cottle also a bit off topic. but your decorators for events idea is really good. as you can see on this projects I was using eventlisteners. https://github.com/byteface/vnc2browser/blob/master/vnc2browser.py and also considered not needing all of domonic just for listeners. but I had not had the decorators idea. nice work.

@Jordan-Cottle
Copy link
Contributor Author

Jordan-Cottle commented Aug 22, 2021

Thanks!

Something you could also do to avoid the "all of Domonic just for X" problem in general would be split up the Dominic packages into their own projects.

Or publish them separately from the same GitHub project (I've not set up something like this before).

That way you can grab the pieces separately if you want.

I think there's also a way to set up a package with optional extras. So you could theoretically set up Domonic to allow selecting it's components (and limit which of it's third party packages are needed depending on what components are needed).

I've not got a lot of experience with publishing packages, so I don't know exactly how to do any of the options beyond just splitting up the package. Just making same observations from how I've seen some of the more complex/larger packages I've used have been set up.

I'll probably have to figure it out for that events package though. The primary goal on my roadmap there is to enable the event publishers and subscribers to be on totally separate machines and that will likely need to involve some kind of server to handle routing event messages. Wouldn't want all the server stuff to be required by every install if most places only need basic http stuff to send/receive messages.

@byteface
Copy link
Owner

ye someone else had suggested me branching the terminal stuff.

I just had a go at forking off javascript to a new package called esx https://github.com/byteface/esx

think ill try seperate out a few concerns. as also helps simplify.

@byteface byteface merged commit 38ab829 into byteface:master Sep 6, 2021
@Jordan-Cottle Jordan-Cottle deleted the add-slots branch September 11, 2021 02:05
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