mboehm7 commented on pull request #1303: URL: https://github.com/apache/systemds/pull/1303#issuecomment-859754787
LGTM. Overall, I think this map is fine - generating a unique token, storing the WTree globally under this token, and compiling the token into the compression instruction for later access is (in my opinion) the best option here. We could further make the WTree part of the normal explain output - that way it's not much different from our dictionaries of functions that are shown in the explain after compilation and then called from multiple different places. Some minor additional comments: * The rework of the WTree seem to have lost the begin/end line information (for the explain output). Especially in large scripts like GLM with >1000 lines and many removed statement blocks during compilation, this information is crucial for effective debugging. * The global map for token lookups could be hardened for concurrent access by different threads (e.g., JMLC) and be made part of the cleanup logic to prevent resource leaks in programmatic APIs. Furthermore, there is no need to create singleton instances of this map - instead it could be a simple class with a static member variable and static methods (which would avoid unnecessary indirections). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
