170045 2017-03-23 22:05:54 -0700 [JSC] Use WeakRandom for SamplingProfiler interval fluctuation 2017-03-23 22:31:58 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 ysuzuki ysuzuki buildbot keith_miller mark.lam msaboff saam oldest_to_newest 1290729 0 ysuzuki 2017-03-23 22:05:54 -0700 [JSC] Use WeakRandom for SamplingProfiler interval fluctuation 1290730 1 305264 ysuzuki 2017-03-23 22:07:01 -0700 Created attachment 305264 Patch 1290736 2 305264 mark.lam 2017-03-23 22:18:10 -0700 Comment on attachment 305264 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305264&action=review r=me with comment. > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:279 > + , m_weakRandom(static_cast<unsigned>(randomNumber() * (std::numeric_limits<unsigned>::max() + 1.0))) Why the + 1.0? I don't think it is needed. I'm guessing you're trying to ensure a non-zero seed, but: 1. this does guarantee a non-zero seed because std::numeric_limits<unsigned>::max() + 1 is still 0, and 2. WeakRandom's setSeed() already guards against a 0 seed. 1290737 3 305264 ysuzuki 2017-03-23 22:22:26 -0700 Comment on attachment 305264 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305264&action=review Thanks! >> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:279 >> + , m_weakRandom(static_cast<unsigned>(randomNumber() * (std::numeric_limits<unsigned>::max() + 1.0))) > > Why the + 1.0? I don't think it is needed. I'm guessing you're trying to ensure a non-zero seed, but: > 1. this does guarantee a non-zero seed because std::numeric_limits<unsigned>::max() + 1 is still 0, and > 2. WeakRandom's setSeed() already guards against a 0 seed. Originally, randomNumber() is created by dividing the result (uint32_t) of cryptographicallyRandomNumber() by `std::numeric_limits<unsigned>::max() + 1.0`. Thus, to recover it to uint32_t range, we need to multiply it by `std::numeric_limits<unsigned>::max() + 1.0`. But, maybe, using cryptographicallyRandomNumber() would be good. Why we divide cryptographicallyRandomNumber() with `std::numeric_limits<unsigned>::max() + 1.0` in `randomNumber()` is to make the double value uniformly distributed in [0, 1.0). It does not include 1.0, thus, we should not divide it with `std::numeric_limits<unsigned>::max()`. If cryptographicallyRandomNumber() produces UINT32_MAX, it produces 1.0. I'll change this code to use cryptographicallyRandomNumber() directly. 1290738 4 305264 ysuzuki 2017-03-23 22:24:43 -0700 Comment on attachment 305264 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305264&action=review >>> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:279 >>> + , m_weakRandom(static_cast<unsigned>(randomNumber() * (std::numeric_limits<unsigned>::max() + 1.0))) >> >> Why the + 1.0? I don't think it is needed. I'm guessing you're trying to ensure a non-zero seed, but: >> 1. this does guarantee a non-zero seed because std::numeric_limits<unsigned>::max() + 1 is still 0, and >> 2. WeakRandom's setSeed() already guards against a 0 seed. > > Originally, randomNumber() is created by dividing the result (uint32_t) of cryptographicallyRandomNumber() by `std::numeric_limits<unsigned>::max() + 1.0`. > Thus, to recover it to uint32_t range, we need to multiply it by `std::numeric_limits<unsigned>::max() + 1.0`. > But, maybe, using cryptographicallyRandomNumber() would be good. > > Why we divide cryptographicallyRandomNumber() with `std::numeric_limits<unsigned>::max() + 1.0` in `randomNumber()` is to make the double value uniformly distributed in [0, 1.0). > It does not include 1.0, thus, we should not divide it with `std::numeric_limits<unsigned>::max()`. If cryptographicallyRandomNumber() produces UINT32_MAX, it produces 1.0. > > I'll change this code to use cryptographicallyRandomNumber() directly. Ah, instead, we just construct in a form `m_weakRandom()`. It will automatically use cryptographicallyRandomNumber() as its seed. 1290739 5 ysuzuki 2017-03-23 22:31:58 -0700 Committed r214336: <http://trac.webkit.org/changeset/214336> 305264 2017-03-23 22:07:01 -0700 2017-03-23 22:18:10 -0700 Patch bug-170045-20170324140700.patch text/plain 3537 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjE0MzM0CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBm MDE3MjRhMmQ0N2UzMDY2MDViYzY0NTc2OWQ4NjcxYmU2YjhkNDAyLi44Y2YyM2YwYzdhZWMwMjFh ZTdlM2U2YmY3MGM0NDI3NDE0YTI2OTc2IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxNy0wMy0yMyAgWXVzdWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWls LmNvbT4KKworICAgICAgICBbSlNDXSBVc2UgV2Vha1JhbmRvbSBmb3IgU2FtcGxpbmdQcm9maWxl ciBpbnRlcnZhbCBmbHVjdHVhdGlvbgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTcwMDQ1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgSXQgaXMgdW5uZWNlc3NhcnkgdG8gdXNlIGNyeXB0b2dyYXBoaWNhbGx5 UmFuZG9tTnVtYmVyIGZvciBTYW1wbGluZ1Byb2ZpbGVyCisgICAgICAgIGludGVydmFsIGZsdWN0 dWF0aW9uLiBVc2UgV2Vha1JhbmRvbSBpbnN0ZWFkLgorCisgICAgICAgICogcnVudGltZS9TYW1w bGluZ1Byb2ZpbGVyLmNwcDoKKyAgICAgICAgKEpTQzo6U2FtcGxpbmdQcm9maWxlcjo6U2FtcGxp bmdQcm9maWxlcik6CisgICAgICAgIChKU0M6OlNhbXBsaW5nUHJvZmlsZXI6OnRpbWVyTG9vcCk6 CisgICAgICAgICogcnVudGltZS9TYW1wbGluZ1Byb2ZpbGVyLmg6CisKIDIwMTctMDMtMjMgIE1h cmsgTGFtICA8bWFyay5sYW1AYXBwbGUuY29tPgogCiAgICAgICAgIEFycmF5LnByb3RvdHlwZS5z cGxpY2UgYmVoYXZlcyBpbmNvcnJlY3RseSB3aGVuIHRoZSBWTSBpcyAiaGF2aW5nIGEgYmFkIHRp bWUiLgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvU2FtcGxpbmdQ cm9maWxlci5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TYW1wbGluZ1Byb2Zp bGVyLmNwcAppbmRleCBkYWVhYzk5MzkxOGUzZjk4N2VkYTYxZjM2MjJmOTNmOTFkYzI2MzVkLi4z MWZlMmI0ZGYzNGU3YzhhZTk0MDkzNGQ5Njk3NTQ3OGZjNGZkNjQzIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TYW1wbGluZ1Byb2ZpbGVyLmNwcAorKysgYi9Tb3Vy Y2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TYW1wbGluZ1Byb2ZpbGVyLmNwcApAQCAtNDcsNyAr NDcsNiBAQAogI2luY2x1ZGUgIlN0cm9uZ0lubGluZXMuaCIKICNpbmNsdWRlICJWTS5oIgogI2lu Y2x1ZGUgPHd0Zi9IYXNoU2V0Lmg+Ci0jaW5jbHVkZSA8d3RmL1JhbmRvbU51bWJlci5oPgogI2lu Y2x1ZGUgPHd0Zi9SZWZQdHIuaD4KICNpbmNsdWRlIDx3dGYvdGV4dC9TdHJpbmdCdWlsZGVyLmg+ CiAKQEAgLTI3Nyw2ICsyNzYsNyBAQCBjbGFzcyBDRnJhbWVXYWxrZXIgOiBwdWJsaWMgRnJhbWVX YWxrZXIgewogCiBTYW1wbGluZ1Byb2ZpbGVyOjpTYW1wbGluZ1Byb2ZpbGVyKFZNJiB2bSwgUmVm UHRyPFN0b3B3YXRjaD4mJiBzdG9wd2F0Y2gpCiAgICAgOiBtX3ZtKHZtKQorICAgICwgbV93ZWFr UmFuZG9tKHN0YXRpY19jYXN0PHVuc2lnbmVkPihyYW5kb21OdW1iZXIoKSAqIChzdGQ6Om51bWVy aWNfbGltaXRzPHVuc2lnbmVkPjo6bWF4KCkgKyAxLjApKSkKICAgICAsIG1fc3RvcHdhdGNoKFdU Rk1vdmUoc3RvcHdhdGNoKSkKICAgICAsIG1fdGltaW5nSW50ZXJ2YWwoc3RkOjpjaHJvbm86Om1p Y3Jvc2Vjb25kcyhPcHRpb25zOjpzYW1wbGVJbnRlcnZhbCgpKSkKICAgICAsIG1fdGhyZWFkSWRl bnRpZmllcigwKQpAQCAtMzI4LDcgKzMyOCw3IEBAIHZvaWQgU2FtcGxpbmdQcm9maWxlcjo6dGlt ZXJMb29wKCkKICAgICAgICAgLy8gZmx1Y3R1YXRpb24gaGVyZS4gVGhlIG1haW4gaWRlYSBpcyB0 byBwcmV2ZW50IG91ciB0aW1lciBmcm9tIGJlaW5nIGluIHN5bmMKICAgICAgICAgLy8gd2l0aCBz b21lIHN5c3RlbSBwcm9jZXNzIHN1Y2ggYXMgYSBzY2hlZHVsZWQgY29udGV4dCBzd2l0Y2guCiAg ICAgICAgIC8vIGh0dHA6Ly9wbHYuY29sb3JhZG8uZWR1L3BhcGVycy9teXRrb3dpY3otcGxkaTEw LnBkZgotICAgICAgICBkb3VibGUgcmFuZG9tU2lnbmVkTnVtYmVyID0gKHJhbmRvbU51bWJlcigp ICogMi4wKSAtIDEuMDsgLy8gQSByYW5kb20gbnVtYmVyIGJldHdlZW4gWy0xLCAxKS4KKyAgICAg ICAgZG91YmxlIHJhbmRvbVNpZ25lZE51bWJlciA9IChtX3dlYWtSYW5kb20uZ2V0KCkgKiAyLjAp IC0gMS4wOyAvLyBBIHJhbmRvbSBudW1iZXIgYmV0d2VlbiBbLTEsIDEpLgogICAgICAgICBzdGQ6 OmNocm9ubzo6bWljcm9zZWNvbmRzIHJhbmRvbUZsdWN0dWF0aW9uID0gc3RkOjpjaHJvbm86Om1p Y3Jvc2Vjb25kcyhzdGF0aWNfY2FzdDx1aW50NjRfdD4ocmFuZG9tU2lnbmVkTnVtYmVyICogc3Rh dGljX2Nhc3Q8ZG91YmxlPihtX3RpbWluZ0ludGVydmFsLmNvdW50KCkpICogMC4yMGwpKTsKICAg ICAgICAgc3RkOjp0aGlzX3RocmVhZDo6c2xlZXBfZm9yKG1fdGltaW5nSW50ZXJ2YWwgLSBzdGQ6 Om1pbihtX3RpbWluZ0ludGVydmFsLCBzdGFja1RyYWNlUHJvY2Vzc2luZ1RpbWUpICsgcmFuZG9t Rmx1Y3R1YXRpb24pOwogICAgIH0KZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9y dW50aW1lL1NhbXBsaW5nUHJvZmlsZXIuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1l L1NhbXBsaW5nUHJvZmlsZXIuaAppbmRleCBkNDRiOTk4YzZmZDUxZmFkYmE5NzE0ZGY5ZGM0YTgx ZjE3MTY5OWU1Li45NWE3MTQ0YTU4YzBlZDU2NjA5ZThjYWRkZGY2NWJkNDczNWViYzUxIDEwMDY0 NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TYW1wbGluZ1Byb2ZpbGVyLmgK KysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvU2FtcGxpbmdQcm9maWxlci5oCkBA IC0zNSw2ICszNSw3IEBACiAjaW5jbHVkZSA8d3RmL0xvY2suaD4KICNpbmNsdWRlIDx3dGYvU3Rv cHdhdGNoLmg+CiAjaW5jbHVkZSA8d3RmL1ZlY3Rvci5oPgorI2luY2x1ZGUgPHd0Zi9XZWFrUmFu ZG9tLmg+CiAKIG5hbWVzcGFjZSBKU0MgewogCkBAIC0xOTAsNiArMTkxLDcgQEAgY2xhc3MgU2Ft cGxpbmdQcm9maWxlciA6IHB1YmxpYyBUaHJlYWRTYWZlUmVmQ291bnRlZDxTYW1wbGluZ1Byb2Zp bGVyPiB7CiAgICAgdm9pZCB0YWtlU2FtcGxlKGNvbnN0IEFic3RyYWN0TG9ja2VyJiwgc3RkOjpj aHJvbm86Om1pY3Jvc2Vjb25kcyYgc3RhY2tUcmFjZVByb2Nlc3NpbmdUaW1lKTsKIAogICAgIFZN JiBtX3ZtOworICAgIFdlYWtSYW5kb20gbV93ZWFrUmFuZG9tOwogICAgIFJlZlB0cjxTdG9wd2F0 Y2g+IG1fc3RvcHdhdGNoOwogICAgIFZlY3RvcjxTdGFja1RyYWNlPiBtX3N0YWNrVHJhY2VzOwog ICAgIFZlY3RvcjxVbnByb2Nlc3NlZFN0YWNrVHJhY2U+IG1fdW5wcm9jZXNzZWRTdGFja1RyYWNl czsK