{"action":"created","issue":{"url":"https://api.github.com/repos/PHPOffice/PhpSpreadsheet/issues/823","repository_url":"https://api.github.com/repos/PHPOffice/PhpSpreadsheet","labels_url":"https://api.github.com/repos/PHPOffice/PhpSpreadsheet/issues/823/labels{/name}","comments_url":"https://api.github.com/repos/PHPOffice/PhpSpreadsheet/issues/823/comments","events_url":"https://api.github.com/repos/PHPOffice/PhpSpreadsheet/issues/823/events","html_url":"https://github.com/PHPOffice/PhpSpreadsheet/issues/823","id":392253060,"node_id":"MDU6SXNzdWUzOTIyNTMwNjA=","number":823,"title":"Cache prefix should/could be optional","user":{"login":"yuloh","id":9440455,"node_id":"MDQ6VXNlcjk0NDA0NTU=","avatar_url":"https://avatars0.githubusercontent.com/u/9440455?v=4","gravatar_id":"","url":"https://api.github.com/users/yuloh","html_url":"https://github.com/yuloh","followers_url":"https://api.github.com/users/yuloh/followers","following_url":"https://api.github.com/users/yuloh/following{/other_user}","gists_url":"https://api.github.com/users/yuloh/gists{/gist_id}","starred_url":"https://api.github.com/users/yuloh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yuloh/subscriptions","organizations_url":"https://api.github.com/users/yuloh/orgs","repos_url":"https://api.github.com/users/yuloh/repos","events_url":"https://api.github.com/users/yuloh/events{/privacy}","received_events_url":"https://api.github.com/users/yuloh/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-12-18T17:10:15Z","updated_at":"2018-12-18T18:05:25Z","closed_at":null,"author_association":"NONE","body":"This is:\r\n\r\n```\r\n- [ ] a bug report\r\n- [x] a feature request\r\n- [ ] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)\r\n```\r\n\r\n### What is the expected behavior?\r\n\r\nThe cell cache does not require a prefix.\r\n\r\n### What is the current behavior?\r\n\r\nAll coordinates are prefixed automatically by `PhpOffice\\PhpSpreadsheet\\Collection\\Cells`\r\n\r\n### What are the steps to reproduce?\r\n\r\nPlease provide a [Minimal, Complete, and Verifiable example](https://stackoverflow.com/help/mcve) of code that exhibits the issue without relying on an external Excel file or a web server:\r\n\r\n```php\r\n<?php\r\n\r\nrequire __DIR__ . '/vendor/autoload.php';\r\n\r\nclass Memory extends \\PhpOffice\\PhpSpreadsheet\\Collection\\Memory\r\n{\r\n    public function set($key, $value, $ttl = null)\r\n    {\r\n        fwrite(STDOUT, \"Cache key: {$key}\\n\"); // prints something like \"Cache key: phpspreadsheet.5c19219c3c4962.46848305.A1\"\r\n        return parent::set($key, $value, $ttl);\r\n    }\r\n}\r\n\r\n\\PhpOffice\\PhpSpreadsheet\\Settings::setCache(new Memory());\r\n\r\n$spreadsheet = new \\PhpOffice\\PhpSpreadsheet\\Spreadsheet();\r\n$spreadsheet->getActiveSheet()->setCellValue('A1', 'foo')->setCellValue('A2', 'bar');\r\n```\r\n\r\n\r\n### Which versions of PhpSpreadsheet and PHP are affected?\r\n\r\nAny version >= 1.0.0-beta\r\n\r\n### Description\r\n\r\nThe class `PhpOffice\\PhpSpreadsheet\\Collection\\Cells` automatically adds a cache prefix to every key before adding it to the cache.  The prefix looks like `phpspreadsheet.5c19219c3c4962.46848305.`.\r\n\r\nThe problem with prefixing is it ends up requiring a lot more memory if the cache adapter stores the keys in memory.  The immediately obvious issue is that we have to store the extra 39 characters for the prefix, repeated by the number of cells in the spreadsheet.  However the memory usage is even worse because `Cells` already [stores an index of every coordinate](https://github.com/PHPOffice/PhpSpreadsheet/blob/294ba58ddef29907667f535d47d2dcc2f1aa8b8b/src/PhpSpreadsheet/Collection/Cells.php#L51).  If the cache keys were not prefixed PHP could reference the same value for both `Cells::index` and the Cache implementation's index.  Since the prefix is added PHP stores each coordinate twice, once without the prefix and once with the prefix.\r\n\r\nI think this could be avoided by letting the cache determine if it wants to add a prefix or not.  Then the cache adapter could either namespace the keys itself or the user could use a decorator [like this one](https://www.php-cache.com/en/latest/namespace/).  It seems like the intention of the prefix is to avoid collisions and the cache should know best if it needs to worry about that.\r\n\r\nWhen using the in memory cache you could avoid collisions by creating a new instance of the memory cache per worksheet.  This would require changing [Settings::getCache](https://github.com/PHPOffice/PhpSpreadsheet/blob/294ba58ddef29907667f535d47d2dcc2f1aa8b8b/src/PhpSpreadsheet/Settings.php#L125) to not force a singleton since it currently returns a single cache instance for all worksheets.  We would probably need to let the user specify a factory instead?\r\n\r\nI tried working around this by making the cache adapter index the cache keys in a multidimensional array as `[prefix][coordinate]`.  It helped memory usage but hurt performance because of all of the string operations necessary.\r\n\r\nI benchmarked performance without prefixing and saw a 21% improvement in runtime and 22% improvement in memory usage.  However there is only a 8.63% improvement in runtime and 14% improvement in memory usage if the changes in #822 are made first.\r\n\r\n- [baseline](https://blackfire.io/profiles/914a6eb7-434e-4a98-99fb-077fe9dee6f8/graph)\r\n- [without cache prefix](https://blackfire.io/profiles/e3589733-81c7-4662-9565-ad4e082d29e9/graph)\r\n- [baseline -> without cache prefix](https://blackfire.io/profiles/compare/277267bf-b9e5-4624-80dc-8a6ebdf4aeb5/graph)\r\n- [#822 -> without cache prefix](https://blackfire.io/profiles/compare/2f29e494-3d8f-46d4-87dd-80a1f8fab4d5/graph)"},"comment":{"url":"https://api.github.com/repos/PHPOffice/PhpSpreadsheet/issues/comments/448314274","html_url":"https://github.com/PHPOffice/PhpSpreadsheet/issues/823#issuecomment-448314274","issue_url":"https://api.github.com/repos/PHPOffice/PhpSpreadsheet/issues/823","id":448314274,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0ODMxNDI3NA==","user":{"login":"yuloh","id":9440455,"node_id":"MDQ6VXNlcjk0NDA0NTU=","avatar_url":"https://avatars0.githubusercontent.com/u/9440455?v=4","gravatar_id":"","url":"https://api.github.com/users/yuloh","html_url":"https://github.com/yuloh","followers_url":"https://api.github.com/users/yuloh/followers","following_url":"https://api.github.com/users/yuloh/following{/other_user}","gists_url":"https://api.github.com/users/yuloh/gists{/gist_id}","starred_url":"https://api.github.com/users/yuloh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yuloh/subscriptions","organizations_url":"https://api.github.com/users/yuloh/orgs","repos_url":"https://api.github.com/users/yuloh/repos","events_url":"https://api.github.com/users/yuloh/events{/privacy}","received_events_url":"https://api.github.com/users/yuloh/received_events","type":"User","site_admin":false},"created_at":"2018-12-18T18:05:25Z","updated_at":"2018-12-18T18:05:25Z","author_association":"NONE","body":"Not in my benchmark script.  It wouldn't have worked correctly because Settings is currently using a single cache entry for every spreadsheet.  I outlined some of my thoughts for working around that in the description above."}}
{"actor":{"display_login":"yuloh"}}