{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/node/issues/24950","repository_url":"https://api.github.com/repos/nodejs/node","labels_url":"https://api.github.com/repos/nodejs/node/issues/24950/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/node/issues/24950/comments","events_url":"https://api.github.com/repos/nodejs/node/issues/24950/events","html_url":"https://github.com/nodejs/node/pull/24950","id":389538080,"node_id":"MDExOlB1bGxSZXF1ZXN0MjM3NTA2NDkw","number":24950,"title":"src: always compile and store code cache for native modules","user":{"login":"joyeecheung","id":4299420,"node_id":"MDQ6VXNlcjQyOTk0MjA=","avatar_url":"https://avatars0.githubusercontent.com/u/4299420?v=4","gravatar_id":"","url":"https://api.github.com/users/joyeecheung","html_url":"https://github.com/joyeecheung","followers_url":"https://api.github.com/users/joyeecheung/followers","following_url":"https://api.github.com/users/joyeecheung/following{/other_user}","gists_url":"https://api.github.com/users/joyeecheung/gists{/gist_id}","starred_url":"https://api.github.com/users/joyeecheung/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/joyeecheung/subscriptions","organizations_url":"https://api.github.com/users/joyeecheung/orgs","repos_url":"https://api.github.com/users/joyeecheung/repos","events_url":"https://api.github.com/users/joyeecheung/events{/privacy}","received_events_url":"https://api.github.com/users/joyeecheung/received_events","type":"User","site_admin":false},"labels":[{"id":155267502,"node_id":"MDU6TGFiZWwxNTUyNjc1MDI=","url":"https://api.github.com/repos/nodejs/node/labels/C++","name":"C++","color":"e11d21","default":false},{"id":281232761,"node_id":"MDU6TGFiZWwyODEyMzI3NjE=","url":"https://api.github.com/repos/nodejs/node/labels/lib%20/%20src","name":"lib / src","color":"fad8c7","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-12-10T23:54:15Z","updated_at":"2018-12-11T00:47:51Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/nodejs/node/pulls/24950","html_url":"https://github.com/nodejs/node/pull/24950","diff_url":"https://github.com/nodejs/node/pull/24950.diff","patch_url":"https://github.com/nodejs/node/pull/24950.patch"},"body":"#### src: remove code cache integrity check\r\n\r\nIn preparation of sharing code cache among different threads -\r\nwe simply rely on v8 to reject invalid cache, since there isn't\r\nany serious consequence when the cache is invalid anyway.\r\n\r\n#### src: always compile and store code cache for native modules\r\n\r\nThis patch changes the NativeModuleLoader to always try to find\r\ncode cache for native modules when it compiles them, and always\r\nproduce and store the code cache after compilation. The cache\r\nmap is protected by a mutex and can be accessed by different\r\nthreads - including the worker threads and the main thread. Hence any\r\nthread can reuse the code cache if the native module has already\r\nbeen compiled by another thread - in particular the cache of the\r\nbootstrappers and per_context.js will always be hit when a new thread\r\nis spun.\r\n\r\nThis results in a ~6% startup overhead in the worst case\r\n(when only the main thread is launched without requiring any additional\r\nnative module - it now needs to do the extra work of finding and\r\nstoring caches), which balances out the recent improvements by moving\r\nthe compilation to C++, but it also leads to a ~60% improvement in\r\nthe best case (when a worker thread is spun and requires a lot of native\r\nmodules thus hitting the cache compiled by the main thread).\r\n\r\nLocal benchmark results:\r\n\r\n```\r\n                                                                                   confidence improvement accuracy (*)   (**)  (***)\r\n misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1        ***     -6.80 %       ±0.77% ±1.02% ±1.34%\r\n misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***     -4.90 %       ±0.82% ±1.10% ±1.43%\r\n misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1         ***     63.13 %       ±1.58% ±2.12% ±2.79%\r\n misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                     ***     56.20 %       ±1.33% ±1.78% ±2.33%\r\n```\r\n\r\n##### Checklist\r\n<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->\r\n\r\n- [x] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes\r\n- [x] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines)\r\n"},"comment":{"url":"https://api.github.com/repos/nodejs/node/issues/comments/446030545","html_url":"https://github.com/nodejs/node/pull/24950#issuecomment-446030545","issue_url":"https://api.github.com/repos/nodejs/node/issues/24950","id":446030545,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0NjAzMDU0NQ==","user":{"login":"joyeecheung","id":4299420,"node_id":"MDQ6VXNlcjQyOTk0MjA=","avatar_url":"https://avatars0.githubusercontent.com/u/4299420?v=4","gravatar_id":"","url":"https://api.github.com/users/joyeecheung","html_url":"https://github.com/joyeecheung","followers_url":"https://api.github.com/users/joyeecheung/followers","following_url":"https://api.github.com/users/joyeecheung/following{/other_user}","gists_url":"https://api.github.com/users/joyeecheung/gists{/gist_id}","starred_url":"https://api.github.com/users/joyeecheung/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/joyeecheung/subscriptions","organizations_url":"https://api.github.com/users/joyeecheung/orgs","repos_url":"https://api.github.com/users/joyeecheung/repos","events_url":"https://api.github.com/users/joyeecheung/events{/privacy}","received_events_url":"https://api.github.com/users/joyeecheung/received_events","type":"User","site_admin":false},"created_at":"2018-12-11T00:47:51Z","updated_at":"2018-12-11T00:47:51Z","author_association":"MEMBER","body":"@addaleax Thanks for the review, updated. Also updated the `emplace()` calls since gcc 4.9.4 needs the pointers to be moved before the call, and updated the test to pass the `--worker` test suit since `compileWithCache` is no longer guaranteed to be empty when a worker thread is spun (that's the whole point of this patch :p)\r\n\r\nPTAL.\r\n\r\nAlso cc @nodejs/process @nodejs/build-files \r\n\r\nCI: https://ci.nodejs.org/job/node-test-pull-request/19412/\r\nBenchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/282/"}}
{"actor":{"display_login":"joyeecheung"}}