{"action":"created","issue":{"url":"https://api.github.com/repos/petkaantonov/bluebird/issues/610","labels_url":"https://api.github.com/repos/petkaantonov/bluebird/issues/610/labels{/name}","comments_url":"https://api.github.com/repos/petkaantonov/bluebird/issues/610/comments","events_url":"https://api.github.com/repos/petkaantonov/bluebird/issues/610/events","html_url":"https://github.com/petkaantonov/bluebird/issues/610","id":73450672,"number":610,"title":"Benchmark: unfair advantage to some implementations","user":{"login":"avih","id":2164962,"avatar_url":"https://avatars.githubusercontent.com/u/2164962?v=3","gravatar_id":"","url":"https://api.github.com/users/avih","html_url":"https://github.com/avih","followers_url":"https://api.github.com/users/avih/followers","following_url":"https://api.github.com/users/avih/following{/other_user}","gists_url":"https://api.github.com/users/avih/gists{/gist_id}","starred_url":"https://api.github.com/users/avih/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/avih/subscriptions","organizations_url":"https://api.github.com/users/avih/orgs","repos_url":"https://api.github.com/users/avih/repos","events_url":"https://api.github.com/users/avih/events{/privacy}","received_events_url":"https://api.github.com/users/avih/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2015-05-05T22:24:34Z","updated_at":"2015-05-06T05:55:13Z","closed_at":"2015-05-06T05:24:20Z","body":"The benchmark results depend immensely on the performance of the `denodeify` implementation - results can change by an order of magnitude depending on the performance of `denodeify` (this also holds true for the `.all` implementations, though to a lesser extent).\r\n\r\nHowever, not all Promise implementations which are being tested at the benchmark implement this functionality internally, and for those, Bluebird benchmark includes a \"fake\" implementation at `benchmark/lib/fakesP.js`.\r\n\r\nThis file also includes a fallback implementation as:\r\n```\r\nelse {\r\n    var lifter = require('when/node').lift;\r\n}\r\n```\r\nwhich is used for at least one of the benchmarked implementations: `promises-dfilatov-vow.js` (beyond the obvious `promises-cujojs-when.js`).\r\n\r\nI believe that the benchmark should only test what the library provides, and if the library's API is slightly different than what the code needs, then a wrapper should be used in a way which will not put this library at a disadvantage. However, I believe that the wrapper at `fakesP.js` is very far from providing equal grounds.\r\n\r\nThe following examples are with `parallel`, but a similar pattern also shows with `doxbee`.\r\n\r\nExample:\r\nnormal run of the benchmark on my system using `./bench parallel`:\r\n```\r\nresults for 10000 parallel executions, 1 ms per I/O op\r\n\r\nfile                                time(ms)  memory(MB)\r\npromises-bluebird.js                     359      109.33\r\npromises-bluebird-generator.js           422      113.92\r\npromises-cujojs-when.js                  594      164.51\r\npromises-tildeio-rsvp.js                 625      217.22\r\ncallbacks-caolan-async-parallel.js       922      225.85\r\npromises-lvivski-davy.js                 922      280.55\r\npromises-calvinmetcalf-lie.js           1062      380.36\r\ncallbacks-baseline.js                   1093       37.64\r\npromises-dfilatov-vow.js                2187      534.49\r\npromises-ecmascript6-native.js          2391      542.76\r\npromises-then-promise.js                2453      695.89\r\npromises-medikoo-deferred.js            3078      535.80\r\npromises-obvious-kew.js                 4594      963.20\r\n\r\nPlatform info:\r\nWindows_NT 6.3.9600 x64\r\nNode.JS 1.8.1  <-- that's actually io.js v1.8.1\r\nV8 4.1.0.27\r\nIntel(R) Core(TM) i7-4500U CPU @ 1.80GHz × 4\r\n```\r\n\r\nHowever, if we modify `fakesP.js` to always use one implementation for all the libraries with the following patch which changes it from fallback to always:\r\n```diff\r\n-else {\r\n     var lifter = require('when/node').lift;\r\n-}\r\n```\r\n\r\nThen the results look like this (obviously same system and same invocation as above):\r\n```\r\nresults for 10000 parallel executions, 1 ms per I/O op\r\n\r\nfile                                time(ms)  memory(MB)\r\npromises-cujojs-when.js                  563      163.62\r\npromises-lvivski-davy.js                 625      198.71\r\npromises-then-promise.js                 672      218.51\r\npromises-bluebird.js                     891      251.84\r\npromises-bluebird-generator.js           937      255.72\r\ncallbacks-caolan-async-parallel.js       969      225.84\r\npromises-tildeio-rsvp.js                1078      360.25\r\ncallbacks-baseline.js                   1110       37.63\r\npromises-obvious-kew.js                 1203      319.76\r\npromises-calvinmetcalf-lie.js           1750      431.94\r\npromises-dfilatov-vow.js                2157      535.04\r\npromises-medikoo-deferred.js            3359      554.31\r\npromises-ecmascript6-native.js          6188      937.73\r\n```\r\nand suddenly the numbers look very different. Some implementation go up the ladder, while other go down (well, except for `promises-dfilatov-vow.js` and `promises-cujojs-when.js` which were already using this implementation before the patch, though they may still appear now higher or lower because others have moved).\r\n\r\nFor instance:\r\n- `promises-bluebird.js` \"deteriorated\" by a factor of ~2.5 from `359 ms / 109 MB` to `851 ms / 251 MB`\r\n- `promises-then-promise.js` \"improved\" by a factor of ~3.5 from `2453 ms / 696 MB` to `672 ms / 218 MB`\r\n- `promises-obvious-kew.js` \"improved\" by a factor of ~3 from `4600 ms / 963 MB` to `1203 ms / 320 MB`.\r\n\r\nRelative to each other, some of the results changed by a factor of ~10.\r\n\r\nThe results with the patch can actually be considered much more \"fair\", since it makes use of \"external independent\" code (external to all the three libs listed above) as `lifter`, thus putting them on equal grounds.\r\n\r\n(I do realize that of those three, only `kew` has a wrapper implementation at `fakesP.js`, while the other two do seem to provide internal implementation, but the example is to make a point of how the results can change when using the same \"construction\" to test different implementation).\r\n\r\nSo, what does this mean? It means that code outside the benchmarked library greatly affects the numbers which the benchmark reports as the score of that library, and this effect (and its magnitude) is different for different libraries.\r\n\r\nHow can it be improved?\r\n\r\nIMO the best thing to do is to use the absolutely minimal API possible from the libraries (probably what the specifications define and what the `promises-aplus-tests` suit expects - plain Promise API), and then build the whole benchmark construction on top of that. This way, libraries which don't implement `denodify` or `all` will still get exactly the same treatment as those who do - benchmarking of their Promise implementation.\r\n\r\nObviously, not everyone would like this suggestion, since clearly some/many library authors have put a lot of time and effort into improving and fine tuning these features, and with such suggestion this effort might not manifest at all at the benchmark.\r\n\r\nOTOH, authors of libraries which have not implemented those (and possibly others - I didn't examine the benchmark code much) would finally have a benchmark which actually tests only what their library provide, and doesn't make their library \"look bad\" due to code which isn't part of the library.\r\n\r\nThe solution to this is, IMO, a different benchmark for features which are not considered core features of Promises.\r\n\r\nOne (or several) benchmark will build only on top of the common Promise features and will test how good does _that_ work in itself and compared to other libraries, and another benchmark which can test the performance of \"extra\" features. If a library provides this feature, it would make sense to test it.\r\n\r\nBut what does not make much sense IMO, is that the benchmark itself includes wrappers which put some implementations at great disadvantage as far as the benchmark results go."},"comment":{"url":"https://api.github.com/repos/petkaantonov/bluebird/issues/comments/99332512","html_url":"https://github.com/petkaantonov/bluebird/issues/610#issuecomment-99332512","issue_url":"https://api.github.com/repos/petkaantonov/bluebird/issues/610","id":99332512,"user":{"login":"avih","id":2164962,"avatar_url":"https://avatars.githubusercontent.com/u/2164962?v=3","gravatar_id":"","url":"https://api.github.com/users/avih","html_url":"https://github.com/avih","followers_url":"https://api.github.com/users/avih/followers","following_url":"https://api.github.com/users/avih/following{/other_user}","gists_url":"https://api.github.com/users/avih/gists{/gist_id}","starred_url":"https://api.github.com/users/avih/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/avih/subscriptions","organizations_url":"https://api.github.com/users/avih/orgs","repos_url":"https://api.github.com/users/avih/repos","events_url":"https://api.github.com/users/avih/events{/privacy}","received_events_url":"https://api.github.com/users/avih/received_events","type":"User","site_admin":false},"created_at":"2015-05-06T05:55:13Z","updated_at":"2015-05-06T05:55:13Z","body":"> However the point of the benchmark is realism, library without a denodeify implementation is virtually unusable\r\n\r\nThat is SO not true. There are many real world use cases of Promises outside of node and without the need to promisify/denodify, and even more cases where denodeify is not part of any hot loop, and the Promise implementation itself is the thing which runs over and over, while the performance of denodify matters not.\r\n\r\nHowever, this benchmark effectively quietly decides to make denodeify the biggest representative \"practicality\" in Promises, and then slaps some implementation which isn't part of the library, and calls it a day.\r\n\r\nWhile I don't disagree that denodify is indeed a very useful feature, you can't just pretend to compare speed of Promise implementations while in fact a very big part of the score is determined by whatever implementation denodify happens to end with.\r\n\r\nIf you want to compare the performance of denodify, make a benchmark which tests denodify on libraries which provide such implementations, and call it `denodify`.\r\n\r\nBut if you pretend to compare different implementations of promises on equal grounds, you should benchmark only what the libraries provide, and where a wrapper is needed, you should be 110% sure that the wrapper doesn't bias the results.\r\n\r\nThere are many places on the web pointing to this benchmark, and quite a few of them wonder how come `Q` is so slow, while in fact, what they don't know is that it only appears slow due to the nature of the benchmark which is mostly affected by the `denodeify` implementation which the benchmark implements for `Q`.\r\n\r\nIf you take an independent function as a `lifter` for the benchmark, then suddenly everything looks very different, and especially `Q` (and no, I have no relation to `Q`).\r\n\r\nThe benchmarks results are overall highly misleading, and more importantly are hiding the fact that a major part of the score comes from the \"extras\" functions which some libraries provide and from the wrappers for libraries who don't have them - which the benchmark itself provides, while the performance of the actual Promise implementation is only a small part of the score.\r\n\r\nIt's just misleading."}}