Reduce repeated calls of get_immediate_node_signature in cache #4871
Conversation
| @@ -67,6 +67,7 @@ def __init__(self, dynprompt, node_ids, is_changed_cache): | |||
| super().__init__(dynprompt, node_ids, is_changed_cache) | |||
| self.dynprompt = dynprompt | |||
| self.is_changed_cache = is_changed_cache | |||
| self.immediate_node_signature = {} | |||
There was a problem hiding this comment.
We can't persist the signatures in the cache keyed by ID. The whole point of the cache is that it persists between executions and there's no guarantee that node signatures will be the same from one execution to the next.
Long term, I think the right solution is to cache entire node definitions somewhere. I have that change locally, but it's grouped with a bunch of others and I won't be able to split it out until this weekend.
Shorter term, if you just reset the .immediate_node_signature at the beginning of each execution, that should work.
There was a problem hiding this comment.
My current hot fix right now is caching I/O calls within folder_paths.py (i.e os.path.*), which the I/O cache can reset whenever object_info api are called and it seems to work so far.
There was a problem hiding this comment.
We can't persist the signatures in the cache keyed by ID. The whole point of the cache is that it persists between executions and there's no guarantee that node signatures will be the same from one execution to the next.
Long term, I think the right solution is to cache entire node definitions somewhere. I have that change locally, but it's grouped with a bunch of others and I won't be able to split it out until this weekend.
Shorter term, if you just reset the
.immediate_node_signatureat the beginning of each execution, that should work.
Currently, cache.set_prompt will reinitialize a cache_key_set each time see.
That is, the cache_key_set between each execution is two independent objects. So from the current implementation, .immediate_node_signature is indeed reset for each execution, because the cache_key_set is different.
This variable is added only to avoid repeated get_immediate_node_signature in get_node_signature of one workflow execution, and has nothing to do with other executions.
Of course, it would be better if this call could be further reduced, because the current implementation can only reduce one cache to be executed at a time, and there are also repeated get_immediate_node_signature between caches.outputs and caches.ui.
There was a problem hiding this comment.
My current hot fix right now is caching I/O calls within
folder_paths.py(i.eos.path.*), which the I/O cache can reset wheneverobject_infoapi are called and it seems to work so far.
This is also a good approach. This PR takes into account more time-consuming INPUT_TYPES issues. I think I/O calls within folder_paths.py is just one of the exposed.
mcmonkey4eva
added
User Support
comfyanonymous
added
the
Run-CI-Test
|
(Automated Bot Message) CI Tests are running, you can view the results at https://ci.comfy.org/?branch=4871%2Fmerge |
comfyanonymous
added
Run-CI-Test
|
(Automated Bot Message) CI Tests are running, you can view the results at https://ci.comfy.org/?branch=4871%2Fmerge |
|
Had to revert this because: #4914 |
I found the problem and will submit a PR for another optimization method later. |
…estors in cache (comfyanonymous#4871)" This reverts commit f6b7194.
CacheKeySetInputSignature.get_immediate_node_signatureis called multiple times for the same node as ancestors of different nodes.Most nodes are fine, but when there are time-consuming operations in
class_def.INPUT_TYPES(), such asget_filename_list, the performance ofcache.set_promptbecomes quite bad on machines with poor disk performance.On one of my devices,
set_promptofcaches.outputsandcaches.uitook 500+ms before modification, and it would be worse if there were more time-consuming nodes. This PR reduces the time to 80ms on average