Bug 172800 – [JSC][MIPS] SamplingProfiler::timerLoop() sleeps for 4000+ seconds

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Guillaume Emont]

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.

[Guillaume Emont]

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.

[Saam Barati]

(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"?

[Guillaume Emont]

(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:

[Guillaume Emont]

(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.

[Guillaume Emont]

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.

[Saam Barati]

Yup, the code in ToT clearly looks wrong. We're looking for a +- random range, but we clearly cast to unsigned.

[Saam Barati]

Your proposed change looks right to me.

[Guillaume Emont]

(In reply to Saam Barati from comment #7)
> Your proposed change looks right to me.

Ok, patch upcoming!

[Guillaume Emont]

Created attachment 311715 [details]
Patch

Patch fixing the issue.

[Saam Barati]

Comment on attachment 311715 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 311715 [details]
Patch

Clearing flags on attachment: 311715

Committed r217663: <http://trac.webkit.org/changeset/217663>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.