Fixed #33465 -- Added empty __slots__ to SafeString and SafeData.

[kezabelle]

ticket is 33465

On the off-chance that's accepted, let's at least check that the tests pass everywhere (i.e. that my asserts are robust enough across multiple versions, tbh)

Place your bets on my getting linter errors ... ;)

[felixxm]

@kezabelle Thanks for this patch 👍

[adamchainz]

You wrote on the ticket:

I don't believe this will have any marked performance change

But this does have a small boost, as Python needn't create an empty dict per object. It's tiny, but I can measure it:

In [1]: class without_slots(str):
   ...:     pass

In [2]: %timeit without_slots("thing")
133 ns ± 0.723 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)


In [3]: class with_slots(str):
   ...:     __slots__ = ()

In [4]: %timeit with_slots("thing")
130 ns ± 0.624 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

Thanks for doing this. I hope you can find some other places to add __slots__ that speed things up.

[kezabelle]

But this does have a small boost [...] It's tiny, but I can measure it

Yeah, it was the "marked" part which should've had emphasis on the ticket :) I too see a small difference -- also by exactly 3ns, though mine is 125ns and 122ns ... interesting. Can I just hold in my head that the difference is always around 3ns, I wonder? That'd sure be nice ;)

I've force-pushed and removed the 'testing cpython' tests ... but the Jenkins CI appears to have skipped, only doing the GitHub actions. Shrug? Oh now GitHub has refreshed and they're appearing as pending. Weird.

[smithdc1]

other places to add slots that speed things up.

The Template node classes come to mind. 🤔

[kezabelle]

The Template node classes come to mind. 🤔

Alas, would that it were that simple.

There's a couple of compounding issues around the Node API that preclude a simple path forward there, IIRC. The first is that they use and mutate class attributes, so an empty slots definition can't work because those become read-only attributes. Marking those attributes as members of the slots def also doesn't work (and, cap in hand, I can't recall which bit of the spec says why ...). You could move the vars into __new__ and be mostly backwards compatible because it's not an API change most would use, but you can't really move them into __init__ because it's never been a requirement to call super().__init__, but then the cost for creating a Node may increase (__new__ + any __init__) so you'd have to be offsetting it enough by introducing the slots for all the builtins (and most of the attributes for any given builtin are only looked up once per render anyway, I think).

So, it may be workable somehow, but not easy. You could probably get some of the memory savings that using slots would provide, by introducing __new__ and returning semi-singletons from an object pool, due to the way Nodes work, though.

(Edit: that all said, you have given me an idea of where I might look for separate, semi-related little win, within the same sort of oeuvre, so perhaps we'll see after all)

[felixxm]

@kezabelle Thanks 👍