{"action":"created","issue":{"url":"https://api.github.com/repos/mapnik/mapnik/issues/3900","repository_url":"https://api.github.com/repos/mapnik/mapnik","labels_url":"https://api.github.com/repos/mapnik/mapnik/issues/3900/labels{/name}","comments_url":"https://api.github.com/repos/mapnik/mapnik/issues/3900/comments","events_url":"https://api.github.com/repos/mapnik/mapnik/issues/3900/events","html_url":"https://github.com/mapnik/mapnik/pull/3900","id":325299485,"node_id":"MDExOlB1bGxSZXF1ZXN0MTg5NjY3MTEy","number":3900,"title":"Marker symbolizer: Cache marker images for faster rendering","user":{"login":"Algunenano","id":664253,"node_id":"MDQ6VXNlcjY2NDI1Mw==","avatar_url":"https://avatars2.githubusercontent.com/u/664253?v=4","gravatar_id":"","url":"https://api.github.com/users/Algunenano","html_url":"https://github.com/Algunenano","followers_url":"https://api.github.com/users/Algunenano/followers","following_url":"https://api.github.com/users/Algunenano/following{/other_user}","gists_url":"https://api.github.com/users/Algunenano/gists{/gist_id}","starred_url":"https://api.github.com/users/Algunenano/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Algunenano/subscriptions","organizations_url":"https://api.github.com/users/Algunenano/orgs","repos_url":"https://api.github.com/users/Algunenano/repos","events_url":"https://api.github.com/users/Algunenano/events{/privacy}","received_events_url":"https://api.github.com/users/Algunenano/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-05-22T13:41:48Z","updated_at":"2018-07-02T18:02:23Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/mapnik/mapnik/pulls/3900","html_url":"https://github.com/mapnik/mapnik/pull/3900","diff_url":"https://github.com/mapnik/mapnik/pull/3900.diff","patch_url":"https://github.com/mapnik/mapnik/pull/3900.patch"},"body":"This adds an image cache to speed up the marker symbolizer.\r\n\r\nWhat is does is, if the marker symbolizer is cacheable (for now it just checks that it doesn't use expressions), it caches the associated `svg::path_attributes`. Then the `svg::path_attributes` contain a vector of images associated with that marker (the same marker rendered with `sampling_rate` (8) precision).\r\n\r\nThis PR comes from the [Carto fork](https://github.com/CartoDB/mapnik). Apart from some minor changes, there we also have metrics and a flag to enable or disable it per map (currently it's always on).\r\n\r\nMain drawbacks I see:\r\n* Breaks `const` since the cached is held internally (in the attributes and the symbolizer). We tried having a global cache (as with the marker_cache singleton) but it required to be pretty big (thousands of images) and to use synchronization (mutexes for multithread support) and it'd still work poorly in high contention scenarios. Also, having it associated with the symbolizer means that when the it disappears so do the associated images (no need to hack around a LRU policy).\r\n* Some subpixel changes (check failed tests).\r\n* The `symbolizer` variant grows in size. I haven't seen anywhere something like an array of thousands of symbolizers, so it shouldn't have any impact.\r\n\r\nMain benefits:\r\n* Faster rendering. Here are some measurements from our fork (Response time for the complete stack) requesting tiles with N points with the same style: [cache_off_vs_cache_on.pdf](https://github.com/mapnik/mapnik/files/2026900/cache_off_vs_cache_on.pdf)\r\n\r\n\r\nPlease let me know of any comments or improvements that could be made to have this accepted.\r\n"},"comment":{"url":"https://api.github.com/repos/mapnik/mapnik/issues/comments/401886327","html_url":"https://github.com/mapnik/mapnik/pull/3900#issuecomment-401886327","issue_url":"https://api.github.com/repos/mapnik/mapnik/issues/3900","id":401886327,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMTg4NjMyNw==","user":{"login":"lightmare","id":1419852,"node_id":"MDQ6VXNlcjE0MTk4NTI=","avatar_url":"https://avatars1.githubusercontent.com/u/1419852?v=4","gravatar_id":"","url":"https://api.github.com/users/lightmare","html_url":"https://github.com/lightmare","followers_url":"https://api.github.com/users/lightmare/followers","following_url":"https://api.github.com/users/lightmare/following{/other_user}","gists_url":"https://api.github.com/users/lightmare/gists{/gist_id}","starred_url":"https://api.github.com/users/lightmare/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lightmare/subscriptions","organizations_url":"https://api.github.com/users/lightmare/orgs","repos_url":"https://api.github.com/users/lightmare/repos","events_url":"https://api.github.com/users/lightmare/events{/privacy}","received_events_url":"https://api.github.com/users/lightmare/received_events","type":"User","site_admin":false},"created_at":"2018-07-02T18:02:23Z","updated_at":"2018-07-02T18:02:23Z","author_association":"CONTRIBUTOR","body":"I don't think this can be accepted as is. One problem I see is casting away `const` from a function argument. That's breach of contract.\r\n(aside note: drawing from STL convention of naming things like `std::cref, std::cbegin, ...`, I wouldn't name const-deprived variables `cattr, csym`; rather `mattr, msym` for mutable)\r\n\r\nMore important issue, though, is that `Map` should be safe to use concurrently. Maybe we're not there, yet, but that's the goal (#2516). So a cache has to be either global or in `mutable` members and used in a thread-safe manner.\r\n"}}
{"actor":{"display_login":"lightmare"}}