[23:33:05] TimStarling: morning! I'd like a review of https://gerrit.wikimedia.org/r/#/c/236401/ if you have the time [23:33:46] nice [23:34:03] I saw an email notification about that but it only said 30% speedup [23:34:13] I guess you have improved it further [23:35:47] oh right, you load the whole thing into memory [23:35:57] I guess that would do the trick, yes [23:37:10] do you think that would be a problem? there would still be gains to have by reading bigger chunks, and then having subsequent read()s get data from the internal buffer, until it is exhausted [23:37:58] but reading it in one go is simpler. i guess it's an issue if the library is used to read 4 GB CDB files, but RAM is cheap [23:42:36] the largest l10n CDB file for the current production branch is Malayalam, at 4.2M. [23:42:59] LocalisationCache keeps all object references alive [23:43:08] so it could actually end up using 1GB of RAM [23:47:55] hmmm [23:49:55] since almost no lines of code are the same, maybe it is worth splitting it into two subclasses, and retaining the old code in a separate subclass [23:52:13] for use in situations where reading the whole file into memory would be an issue? yeah, can do. [23:52:28] yes [23:52:48] it means you have to have some way to configure which subclass is used, that is the only difficulty [23:55:30] well, there are already two implementations to choose from (Reader\DBA and Reader\PHP) [23:55:56] yeah, but it is automatic, not configured [23:56:55] maybe a $options parameter to Reader::open() [23:57:17] but I would be inclined to default to the low-memory one [23:58:34] since small installations using the default config are more likely to be memory constrained, and more likely to have severe consequences (e.g. downtime) if memory limits are exceeded [23:59:31] yes, agreed. And the library is decoupled from MediaWiki now, so it is doubly important to have the default implementation work