Cache repeated COLR subtables (2) #3221
Conversation
otherwise we'd confuse two objects with distinct classes that happen to have the same 'source'
maybe to choose the default cache size we need more experiments with real-world fonts
|
Using paintcompiler on Bitcount: FontTools main: 2:19.07 (peak memory usage 4.8G) Probably better to do nothing than to do this. |
|
well.. that's strange, how come your branch where we don't manage the cache at all and let it grow unbounded actually reports using less memory? How did you measure? |
|
Don't know. The TTX is different between the master build and my build, but visually it's the same. I'm measuring the memory footprint with "top", which I know isn't completely accurate, but it's not going to be an order of magnitude wrong either. |
|
if the ttx is different it must be a bug, because this optimization is only meant to speed things up but the end result ought to be identical |
| # Move to the end to mark it as most recently used | ||
| self.cache.move_to_end(cacheKey) | ||
| # return unique copies; pickle/unpickle is faster than copy.deepcopy | ||
| return pickle.loads(pickle.dumps(self.cache[cacheKey])) |
There was a problem hiding this comment.
This is ugly, even if faster than deepcopy. :)
There was a problem hiding this comment.
Also, you might get away without a copy whatsoever, no?
There was a problem hiding this comment.
This is ugly, even if faster than deepcopy. :)
the whole point of this was to speed things up...
you might get away without a copy whatsoever, no?
I don't think so. If you use this to build master COLR tables that are then sent to varLib.merger to build variable COLR, the thing is updated in-place and you don't want shared objects. When decompiling otTables from binary, you do get distinct objects even when they were pointed to by same offset.
Some improvements to #3215 to use pickle (faster than json), sha256 for the digest to mitigate collisions, and a LRU cache to avoid it growing unbounded.
I'm still unsure regarding the cache max size (I used 128 like lru_cache but don't know what a good number is in general for this task).
I'm also hesitant to enable this unconditionally and would rather it be opt in, as it's not guaranteed to lead to faster builds given the overhead of pickling+hashing objects mutliple times (the TableBuilder.build is called recursively for each struct fields in a table tree).