[FIRRTL] Dedup: speed up handling of instances #7815
Conversation
Dedup tries to hash all modules in parallel. To accomplish this, the names of instantiated modules are not included as part of the structural hash, but they are taken in to account when checking if two modules are the same. This process involves comparing the instantiated children modules of two modules if their hashes match. This was implemented by using an array attribute, to make comparisons quicker. When a module or class has many thousands of instances underneath it, it becomes impractical to build a array attribute with every child module. Interning such a large ArrayAttr is incredibly slow and will eat up that memory for the rest of the process. Instead, we don't bother interning the instance arrays, and just keep them as plain old vectors, which comes with the benefit of not eagerly interning gigantic arrays.
| }; | ||
|
|
||
| static bool operator==(const ModuleInfo &lhs, const ModuleInfo &rhs) { | ||
| return lhs.structuralHash == rhs.structuralHash && | ||
| lhs.referredModuleNames == rhs.referredModuleNames; |
There was a problem hiding this comment.
Is there any chance this vector comparison rather expensive than interning them as ArrayAttr? Not sure how common it is but if there are modules that has same hash and instances within them refer to different modules, it could be possible that this comparison occurs multiple times.
There was a problem hiding this comment.
Yeah, its possible it does the comparison multiple times. I was thinking that the same comparison problem could happen when creating the ArrayAttr by interning the vector in a DenseSet, so its probably similar in the worst case of both. I just noticed that StorageUniquer stores a key's hash as part of the key, which seems like it could help reduce expensive comparisons 🤔
| @@ -89,9 +90,14 @@ struct ModuleInfo { | |||
| // SHA256 hash. | |||
| std::array<uint8_t, 32> structuralHash; | |||
| // Module names referred by instance op in the module. | |||
| mlir::ArrayAttr referredModuleNames; | |||
| std::vector<StringAttr> referredModuleNames; | |||
There was a problem hiding this comment.
SmallVector should be fine for these.
| @@ -359,7 +363,7 @@ struct StructuralHasher { | |||
| DenseMap<StringAttr, SymbolTarget> innerSymTargets; | |||
|
|
|||
| // This keeps track of module names in the order of the appearance. | |||
| SmallVector<mlir::StringAttr> referredModuleNames; | |||
| std::vector<StringAttr> referredModuleNames; | |||
There was a problem hiding this comment.
SmallVector should still be fine for these.
Dedup tries to hash all modules in parallel. To accomplish this, the names of instantiated modules are not included as part of the structural hash, but they are taken in to account when checking if two modules are the same. This process involves comparing the instantiated children modules of two modules if their hashes match. This was implemented by using an array attribute, to make comparisons quicker.
When a module or class has many thousands of instances underneath it, it becomes impractical to build a array attribute with every child module. Interning such a large ArrayAttr is incredibly slow and will eat up that memory for the rest of the process.
Instead, we don't bother interning the instance arrays, and just keep them as plain old vectors, which comes with the benefit of not eagerly interning gigantic arrays.