[JSC][GTK] glib RunLoop does not accept negative start interval
Created attachment 306915 [details] Patch
What is trying to use a negative start interval? Why should that not result in an assertion?
(In reply to Michael Catanzaro from comment #2) > What is trying to use a negative start interval? Why should that not result > in an assertion? In the GCActivityCallback timer, we usually repeatedly invokes the callback with decade, but sometime we would like to invoke the callback soon. In the code, we calculate the delta in scheduleTimer. But above calculation can result negative interval if enough time is already spent. It is valid. In such a case, we should schedule the timer with 0_s. This patch just does so. Previously, we directly use glib timer, which does not have any assertion with negative delays. But now, we start using RunLoop::Timer, which has such an assertion. Thus we should have this patch.
Ping?
Comment on attachment 306915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306915&action=review > Source/JavaScriptCore/heap/GCActivityCallback.cpp:92 > + m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s)); Why is this needed?
Comment on attachment 306915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306915&action=review >> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92 >> + m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s)); > > Why is this needed? For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative.
Comment on attachment 306915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306915&action=review >>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92 >>> + m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s)); >> >> Why is this needed? > > For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative. Why not remove that assumption since it's wrong? Seems cleaner to me. Numbers less than current time happen for very natural reasons in code.
Comment on attachment 306915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306915&action=review >>>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92 >>>> + m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s)); >>> >>> Why is this needed? >> >> For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative. > > Why not remove that assumption since it's wrong? Seems cleaner to me. Numbers less than current time happen for very natural reasons in code. I think `startOneShot(negative)` is ok. But I'm worried about that it becomes a bit ugly semantics when getting `startRepeating(negative)`.
Comment on attachment 306915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306915&action=review r=me >>>>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92 >>>>> + m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s)); >>>> >>>> Why is this needed? >>> >>> For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative. >> >> Why not remove that assumption since it's wrong? Seems cleaner to me. Numbers less than current time happen for very natural reasons in code. > > I think `startOneShot(negative)` is ok. But I'm worried about that it becomes a bit ugly semantics when getting `startRepeating(negative)`. Ok. It seems reasonable to me to make the change in startOneShot, but I'll leave it up to you
Could we land this now and leave the improvement over startOneShot() semantics for later? The GTK debug builds are really broken currently.
Comment on attachment 306915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306915&action=review >>>>>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92 >>>>>> + m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s)); >>>>> >>>>> Why is this needed? >>>> >>>> For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative. >>> >>> Why not remove that assumption since it's wrong? Seems cleaner to me. Numbers less than current time happen for very natural reasons in code. >> >> I think `startOneShot(negative)` is ok. But I'm worried about that it becomes a bit ugly semantics when getting `startRepeating(negative)`. > > Ok. It seems reasonable to me to make the change in startOneShot, but I'll leave it up to you OK, I'll do the both. While accepting negative seconds is ok, I think leaving `std::max<Seconds>()` is good annotation that this may produce negative values. Since we do not hit that assertion before, it means that all the other places simply uses positive value, it is good.
Note that GSource actually accepts negative values, but with a different meaning. It only accepts -1 to disable the source, which means never fire this. If negative values are allowed by RunLoop in general, and mean fire ASAP, we should handle that case in the runLoop glib implementation by using 0 in such cases.
Committed r215454: <http://trac.webkit.org/changeset/215454>
(In reply to Carlos Garcia Campos from comment #12) > Note that GSource actually accepts negative values, but with a different > meaning. It only accepts -1 to disable the source, which means never fire > this. If negative values are allowed by RunLoop in general, and mean fire > ASAP, we should handle that case in the runLoop glib implementation by using > 0 in such cases. Right. Landed one does so. ;)
*** Bug 170893 has been marked as a duplicate of this bug. ***