youtube / spitfire

A high-performance Python template language
BSD 3-Clause "New" or "Revised" License
403 stars 59 forks source link

Eliminate a reference cycle caused by the Spitfire placeholder_cache. #67

Closed ryankennedy closed 7 years ago

ryankennedy commented 7 years ago

Uses weakref.proxy() when adding methods to the cache in order to avoid reference cycles.

ryankennedy commented 7 years ago

Added a test and then added a fix for the issue you found.

shalabhc commented 7 years ago

I'm curious about the performance impact of this change as resolve_placeholder is usually a hot function. Are there any benchmarks?

ryankennedy commented 7 years ago

I ran the benchmark scripts a few times and didn't notice any glaring differences.

shalabhc commented 7 years ago

I see. Does it include methods specifically? Maybe it's not be worth caching self.methods at all - if a getattr() is faster than an isinstance()+weakref().