172800 2017-06-01 03:35:31 -0700 [JSC][MIPS] SamplingProfiler::timerLoop() sleeps for 4000+ seconds 2017-06-01 10:42:28 -0700 1 1 1 Unclassified WebKit JavaScriptCore Other Other Linux RESOLVED FIXED P2 Normal --- 1 guijemont webkit-unassigned buildbot commit-queue guijemont joepeck keith_miller mark.lam msaboff saam oldest_to_newest 1314567 0 guijemont 2017-06-01 03:35:31 -0700 When running stress/sampling-profiler-basic.js, the value of randomFluctuation in SamplingProfiler::timerLoop() can get pretty large, meaning the sampling profiler does not sample much. This causes a bunch of sampling-profiler tests in stress/ to fail. 1314568 1 guijemont 2017-06-01 03:36:11 -0700 It looks like changing the cast from uint64_t to int64_t solves the issue, but I'm not sure it is the correct approach. 1314643 2 saam 2017-06-01 09:24:35 -0700 (In reply to Guillaume Emont from comment #1) > It looks like changing the cast from uint64_t to int64_t solves the issue, > but I'm not sure it is the correct approach. Interesting! uint64_t to int64_t where? Also, what do you mean by "pretty large"? 1314658 3 guijemont 2017-06-01 09:52:51 -0700 (In reply to Saam Barati from comment #2) > (In reply to Guillaume Emont from comment #1) > > It looks like changing the cast from uint64_t to int64_t solves the issue, > > but I'm not sure it is the correct approach. > > Interesting! > > uint64_t to int64_t where? Also, what do you mean by "pretty large"? Sorry, I could have been more precise. I mean to change: std::chrono::microseconds randomFluctuation = std::chrono::microseconds(static_cast<uint64_t>(randomSignedNumber * static_cast<double>(m_timingInterval.count()) * 0.20l)); into: 1314664 4 guijemont 2017-06-01 09:54:37 -0700 (In reply to Saam Barati from comment #2) > (In reply to Guillaume Emont from comment #1) > > It looks like changing the cast from uint64_t to int64_t solves the issue, > > but I'm not sure it is the correct approach. > > Interesting! > > uint64_t to int64_t where? Also, what do you mean by "pretty large"? Sorry, I could have been more precise. I meant to change: std::chrono::microseconds randomFluctuation = std::chrono::microseconds(static_cast<uint64_t>(randomSignedNumber * static_cast<double>(m_timingInterval.count()) * 0.20l)); into: std::chrono::microseconds randomFluctuation = std::chrono::microseconds(static_cast<int64_t>(randomSignedNumber * static_cast<double>(m_timingInterval.count()) * 0.20l)); Alternatively, we could put an abs() inside the static_cast<uint64_t>(), depending on what the original intent is. 1314669 5 guijemont 2017-06-01 10:00:51 -0700 And by pretty large, I mean that sleep afterwards was of more than 4000 seconds (so I guess the value was somewhere close to 2^32?). If that matters, I can run a new debugging session to see the exact number I get. My guess is that the static_cast from a negative double to an uint64_t is undefined, at least it looks like my compiler (gcc 6.2.0 targeting mipsel) treats it as such. 1314670 6 saam 2017-06-01 10:01:26 -0700 Yup, the code in ToT clearly looks wrong. We're looking for a +- random range, but we clearly cast to unsigned. 1314672 7 saam 2017-06-01 10:02:59 -0700 Your proposed change looks right to me. 1314675 8 guijemont 2017-06-01 10:05:15 -0700 (In reply to Saam Barati from comment #7) > Your proposed change looks right to me. Ok, patch upcoming! 1314679 9 311715 guijemont 2017-06-01 10:14:10 -0700 Created attachment 311715 Patch Patch fixing the issue. 1314680 10 311715 saam 2017-06-01 10:15:06 -0700 Comment on attachment 311715 Patch r=me 1314704 11 311715 commit-queue 2017-06-01 10:42:26 -0700 Comment on attachment 311715 Patch Clearing flags on attachment: 311715 Committed r217663: <http://trac.webkit.org/changeset/217663> 1314705 12 commit-queue 2017-06-01 10:42:28 -0700 All reviewed patches have been landed. Closing bug. 311715 2017-06-01 10:14:10 -0700 2017-06-01 10:42:26 -0700 Patch bug-172800-20170601121410.patch text/plain 2107 guijemont U3VidmVyc2lvbiBSZXZpc2lvbjogMjE3NjUwCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBi MDU3ZDA3MWQ5YzU5MjdlNDc2YzRmZjdiNWU3NTQzMmYxMTFiNTFmLi4zYzk0MmQ5YmJjMDI4ZGE2 OWFiYjI5ZTlkYTgzMGFiMzAzYWIxZGQ0IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxNy0wNi0wMSAgR3VpbGxhdW1lIEVtb250ICA8Z3VpamVtb250QGlnYWxp YS5jb20+CisKKyAgICAgICAgW0pTQ11bTUlQU10gU2FtcGxpbmdQcm9maWxlcjo6dGltZXJMb29w KCkgc2xlZXBzIGZvciA0MDAwKyBzZWNvbmRzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xNzI4MDAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBUaGlzIGZpeGVzIGEgc3RhdGljX2Nhc3Q8dWludDY0X3Q+IGJ5 IG1ha2luZyBpdCBhIGNhc3QgdG8gaW50NjRfdAorICAgICAgICBpbnN0ZWFkLCB3aGljaCBsb29r cyBsaWtlIHRoZSBvcmlnaW5hbCBpbnRlbnQuIFRoaXMgZml4ZXMgdGhlCisgICAgICAgIHNhbXBs aW5nLXByb2ZpbGVyIHRlc3RzIGluIEpTVGVzdHMvc3RyZXNzLgorCisgICAgICAgICogcnVudGlt ZS9TYW1wbGluZ1Byb2ZpbGVyLmNwcDoKKyAgICAgICAgKEpTQzo6U2FtcGxpbmdQcm9maWxlcjo6 dGltZXJMb29wKToKKwogMjAxNy0wNi0wMSAgVG9tYXMgUG9wZWxhICA8dHBvcGVsYUByZWRoYXQu Y29tPiwgTWFyayBMYW0gIDxtYXJrLmxhbUBhcHBsZS5jb20+CiAKICAgICAgICAgUkVMRUFTRV9B U1NFUlRfTk9UX1JFQUNIRUQoKSBpbiBJbmZlcnJlZFR5cGU6OmtpbmRGb3JGbGFncygpIG9uIEJp Zy1FbmRpYW5zCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TYW1w bGluZ1Byb2ZpbGVyLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1NhbXBsaW5n UHJvZmlsZXIuY3BwCmluZGV4IDdjZDg3M2UyYjIzMTZiYTYyM2UzMjE2MmFlYTNmOGY2ZmUyNGFi NjIuLjQ2ZmJjZGExZDc5NTQ1YzY5OWNhZGExNjFhMmQwNjkzN2Q2ZDIzMDggMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1NhbXBsaW5nUHJvZmlsZXIuY3BwCisrKyBi L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1NhbXBsaW5nUHJvZmlsZXIuY3BwCkBAIC0z MjUsNyArMzI1LDcgQEAgdm9pZCBTYW1wbGluZ1Byb2ZpbGVyOjp0aW1lckxvb3AoKQogICAgICAg ICAvLyB3aXRoIHNvbWUgc3lzdGVtIHByb2Nlc3Mgc3VjaCBhcyBhIHNjaGVkdWxlZCBjb250ZXh0 IHN3aXRjaC4KICAgICAgICAgLy8gaHR0cDovL3Bsdi5jb2xvcmFkby5lZHUvcGFwZXJzL215dGtv d2ljei1wbGRpMTAucGRmCiAgICAgICAgIGRvdWJsZSByYW5kb21TaWduZWROdW1iZXIgPSAobV93 ZWFrUmFuZG9tLmdldCgpICogMi4wKSAtIDEuMDsgLy8gQSByYW5kb20gbnVtYmVyIGJldHdlZW4g Wy0xLCAxKS4KLSAgICAgICAgc3RkOjpjaHJvbm86Om1pY3Jvc2Vjb25kcyByYW5kb21GbHVjdHVh dGlvbiA9IHN0ZDo6Y2hyb25vOjptaWNyb3NlY29uZHMoc3RhdGljX2Nhc3Q8dWludDY0X3Q+KHJh bmRvbVNpZ25lZE51bWJlciAqIHN0YXRpY19jYXN0PGRvdWJsZT4obV90aW1pbmdJbnRlcnZhbC5j b3VudCgpKSAqIDAuMjBsKSk7CisgICAgICAgIHN0ZDo6Y2hyb25vOjptaWNyb3NlY29uZHMgcmFu ZG9tRmx1Y3R1YXRpb24gPSBzdGQ6OmNocm9ubzo6bWljcm9zZWNvbmRzKHN0YXRpY19jYXN0PGlu dDY0X3Q+KHJhbmRvbVNpZ25lZE51bWJlciAqIHN0YXRpY19jYXN0PGRvdWJsZT4obV90aW1pbmdJ bnRlcnZhbC5jb3VudCgpKSAqIDAuMjBsKSk7CiAgICAgICAgIHN0ZDo6dGhpc190aHJlYWQ6OnNs ZWVwX2ZvcihtX3RpbWluZ0ludGVydmFsIC0gc3RkOjptaW4obV90aW1pbmdJbnRlcnZhbCwgc3Rh Y2tUcmFjZVByb2Nlc3NpbmdUaW1lKSArIHJhbmRvbUZsdWN0dWF0aW9uKTsKICAgICB9CiB9Cg==