perf: cache parsed CSV file data and replace Array with Set to improve code gen performance #168
Conversation
chrispcampbell
changed the title
chrispcampbell
changed the title
| } | ||
| let data = B.read(pathname) | ||
| csv = parseCsv(data, CSV_PARSE_OPTS) | ||
| csvData.set(pathname, csv) |
There was a problem hiding this comment.
I wonder if we need to worry about caching many large files in memory being an issue for models that read many large CSV files (but that only make use of parts of them). But I suspect the answer is "probably not", and if we do ever encounter such a beast, we can worry about it at that time.
There was a problem hiding this comment.
@ToddFincannon I was just about to approve and merge this but saw your latest comment in #167. I can either hold off on merging if you think you have other changes to make as part of this PR (i.e., make this PR a general "make EPS code gen faster" thing), or I can merge this without the "fixes" tag so that we leave the issue open and tackle it with multiple PRs. Let me know which you prefer.
|
That optimization reduced the full output genc time by 3x (see #167 for the flame graph). I think it's ready to merge now. I worried briefly about the memory footprint of caching the CSVs, but it doesn't seem to be a problem. In EPS we cache 700 mostly small CSVs, and computers have lots of memory these days anyway. |
chrispcampbell
changed the title
Fixes #167