{"action":"created","issue":{"url":"https://api.github.com/repos/ch11ng/xelb/issues/2","labels_url":"https://api.github.com/repos/ch11ng/xelb/issues/2/labels{/name}","comments_url":"https://api.github.com/repos/ch11ng/xelb/issues/2/comments","events_url":"https://api.github.com/repos/ch11ng/xelb/issues/2/events","html_url":"https://github.com/ch11ng/xelb/pull/2","id":101663272,"number":2,"title":"Improve performance when unmarshalling long vectors","user":{"login":"pipcet","id":4129208,"avatar_url":"https://avatars.githubusercontent.com/u/4129208?v=3","gravatar_id":"","url":"https://api.github.com/users/pipcet","html_url":"https://github.com/pipcet","followers_url":"https://api.github.com/users/pipcet/followers","following_url":"https://api.github.com/users/pipcet/following{/other_user}","gists_url":"https://api.github.com/users/pipcet/gists{/gist_id}","starred_url":"https://api.github.com/users/pipcet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pipcet/subscriptions","organizations_url":"https://api.github.com/users/pipcet/orgs","repos_url":"https://api.github.com/users/pipcet/repos","events_url":"https://api.github.com/users/pipcet/events{/privacy}","received_events_url":"https://api.github.com/users/pipcet/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-08-18T14:02:59Z","updated_at":"2015-08-18T17:42:36Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ch11ng/xelb/pulls/2","html_url":"https://github.com/ch11ng/xelb/pull/2","diff_url":"https://github.com/ch11ng/xelb/pull/2.diff","patch_url":"https://github.com/ch11ng/xelb/pull/2.patch"},"body":"I noticed quite poor performance of `xcb:keysyms:init` (in excess of five seconds to run with the CPU at its slowest speed), and while I realize performance has not been a major concern so far, I believe this is worth fixing. It's due to this code in xcb-types.el:\r\n\r\n            (dotimes (i list-size)\r\n              (setq tmp (xcb:-unmarshal-field obj x data nil))\r\n              (setq result (nconc result (list (car tmp))))\r\n              (setq data (substring data (cadr tmp)))\r\n              (setq count (+ count (cadr tmp))))\r\n\r\nThat's really inefficient if `data` is quite long, because `substring` doesn't appear to perform any optimizations in this case, so we perform O(n²) copying operations.\r\n\r\nThis patch avoids using `substring` and passes around the current offset into the `data` vector instead; that makes the code slightly more complicated, but I believe it is worth it as performance improves drastically. The critical code section turns into:\r\n\r\n            (dotimes (i list-size)\r\n              (setq tmp (xcb:-unmarshal-field obj x data (+ offset count) nil))\r\n              (setq result (nconc result (list (car tmp))))\r\n              (setq count (+ count (cadr tmp))))\r\n\r\nwith no modification of `data`.\r\n\r\nNote that a corresponding change is needed in xelb-util: https://github.com/ch11ng/xelb-util/compare/master...pipcet:data-offset?expand=1\r\n\r\nWhat do you think? I understand if you decide not to apply this patch because the complication isn't worth the performance gain, but I believe that it is."},"comment":{"url":"https://api.github.com/repos/ch11ng/xelb/issues/comments/132292266","html_url":"https://github.com/ch11ng/xelb/pull/2#issuecomment-132292266","issue_url":"https://api.github.com/repos/ch11ng/xelb/issues/2","id":132292266,"user":{"login":"pipcet","id":4129208,"avatar_url":"https://avatars.githubusercontent.com/u/4129208?v=3","gravatar_id":"","url":"https://api.github.com/users/pipcet","html_url":"https://github.com/pipcet","followers_url":"https://api.github.com/users/pipcet/followers","following_url":"https://api.github.com/users/pipcet/following{/other_user}","gists_url":"https://api.github.com/users/pipcet/gists{/gist_id}","starred_url":"https://api.github.com/users/pipcet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pipcet/subscriptions","organizations_url":"https://api.github.com/users/pipcet/orgs","repos_url":"https://api.github.com/users/pipcet/repos","events_url":"https://api.github.com/users/pipcet/events{/privacy}","received_events_url":"https://api.github.com/users/pipcet/received_events","type":"User","site_admin":false},"created_at":"2015-08-18T17:42:36Z","updated_at":"2015-08-18T17:42:36Z","body":"Thanks for responding!\r\n\r\nAbsolutely, take all the time you need, and please let me know if you find any problems so I can fix them and try again. There's also a minimal workaround that changes only the `xcb:KEYSYM` case at https://github.com/ch11ng/xelb/compare/master...pipcet:keysym-workaround?expand=1, but I must admit I really prefer the version that adds the offset argument everywhere.\r\n\r\n(I haven't changed the `xcb:unmarshal` API, just `xcb:-unmarshal-*`; on the one hand, that means no public API change, but on the other hand, it means that extended types will always have a performance problem when used in long vectors. I still think it's early enough to change that, but it's obviously not a decision to be taken lightly. I think it might make most sense to add an `xcb:unmarshal-at` method and switch to that if it becomes necessary.)"}}