Bug 147107 – Implementation JavaScript watchdog using WTF::WorkQueue.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Pascal Jacquemart]

Adding default implementation for JavaScript watchdog based on C++11 features (threading, mutex, condition_variables)

[Pascal Jacquemart]

Created attachment 257092 [details]
Patch

[Pascal Jacquemart]

Too bad it is not compiling for Windows ;-(

I am working on WebKitEFL, trying to call JSContextGroupSetExecutionTimeLimit() from the injected bundle

It works -the JavaScript has been stopped after trying to execute a faulty page running an infinite loop- but... it won't restart!

I mean I can load new pages using the MiniBrowser URL bar but the JavaScript is still disabled. Is it an EFL bug?

[Pascal Jacquemart]

Created attachment 257262 [details]
Patch

[Mark Lam]

Pascal, why did you r+ your own patch?

[Pascal Jacquemart]

Sorry I meant r? of course
How is it possible? I can r+ myself whereas I am not even a reviewer?

[Mark Lam]

Comment on attachment 257262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257262&action=review

> Source/JavaScriptCore/runtime/WatchdogGeneric.cpp:36
> +    unique_lock<std::mutex> lock(m_mutex);

Let's use a "std::lock_guard<std::mutex> lock(m_mutex);" instead since it also communicates that the mutex will not be unlocked until this scope exits.  The unique_lock<std::mutex> implies that you may unlock it mid scope (which you don't).

> Source/JavaScriptCore/runtime/WatchdogGeneric.cpp:47
> +    m_running = false;
> +    m_active = false;
> +    m_cond.notify_all();
> +    m_mutex.unlock();

It is wrong to force unlock the mutex here, especially since you have not locked it.  The right thing to do is to put these 3 lines above this in a scope, and add a "std::lock_guard<std::mutex> lock(m_mutex);" at the top of the scope.

> Source/JavaScriptCore/runtime/WatchdogGeneric.cpp:71
> +    m_timeout = chrono::steady_clock::now() + limit;
> +    m_active = true;
> +    m_cond.notify_all();

You're should add a "std::lock_guard<std::mutex> lock(m_mutex);" at the top of this scope.  Otherwise, you'll have a race condition with Watchdog::run() due to spurious wakes from m_cond.wait().

> Source/JavaScriptCore/runtime/WatchdogGeneric.cpp:77
> +    m_active = false;
> +    m_cond.notify_all();

You're should add a "std::lock_guard<std::mutex> lock(m_mutex);" at the top of this scope.  You might be able to get away without locking in this case here, but let's be pedantic about it so that it is clear that m_active and m_cond are under the guard of m_mutex.

[Mark Lam]

Comment on attachment 257262 [details]
Patch

One more thing: I think you should svn delete WatchdogNone.cpp since it is no longer needed.

[Geoffrey Garen]

Comment on attachment 257262 [details]
Patch

Is there any reason to have two implementations of this feature? Why not just one?

[Mark Lam]

(In reply to comment #8)
> Comment on attachment 257262 [details]
> Patch
> 
> Is there any reason to have two implementations of this feature? Why not
> just one?

Darwin port uses GCD.  The default implementation being proposed creates a thread per WatchDog i.e. per VM.  I believe the GCD solution is more efficient for Darwin, but is unfortunately not always available for other ports.

The watchdog thread solution could be optimized to use only one thread, but that would effectively be like implementing a form of GCD.  If the other ports don't instantiate multiple VMs as a normal pattern of usage, then it may not be worth the effort to work on this optimization (which is why I didn't bring it up in my review).

[Anders Carlsson]

I think the default implementation of this should use WorkQueue (which is a GCD abstraction).

[Mark Lam]

(In reply to comment #10)
> I think the default implementation of this should use WorkQueue (which is a
> GCD abstraction).

Interesting.  That sounds like the right plan, and we can have one implementation after all.  I'll let Pascal work on the implementation.

[Michael Catanzaro]

This was already implemented but ignored by reviewers in bug #121219, but for GTK and EFL only, so one bug or the other should be closed, I think.

[Mark Lam]

(In reply to comment #12)
> This was already implemented but ignored by reviewers in bug #121219, but
> for GTK and EFL only, so one bug or the other should be closed, I think.

It would help to cc me on Watchdog changes.  Anyway, it appears that 121219 is also implementing the same solution of using a monitoring thread.  As Anders pointed out above (https://bugs.webkit.org/show_bug.cgi?id=147107#c10), WTF::WorkQueue provides the abstraction we need to implement the Watchdog in a generic way for all platforms.

Hence, we should refactor the default implementation to be expressed in terms of WTF::WorkQueue, and have all platforms use that instead.

[Michael Catanzaro]

*** Bug 121219 has been marked as a duplicate of this bug. ***

[Pascal Jacquemart]

Created attachment 257832 [details]
Patch

[Pascal Jacquemart]

Comment on attachment 257832 [details]
Patch

Not asking for review, just saving the work in progress...

Thanks Mark for you comment on Bug 131082, it seems I missed the obvious...
Calling JSContextGroupSetExecutionTimeLimit() from a WebExtension / injected bundle is working fine... while using the appropriate callback.

In this concern "load_started" is fine because it is calling JSContextGroupSetExecutionTimeLimit() for each page load hence reseting m_didFire state

I can try to use WTF::WorkQueue, it sounds great!
Actually it seems we could apply the same treatment to remove / refactor heap/HeapTimer.cpp/.h

[Mark Lam]

(In reply to comment #16)
> Comment on attachment 257832 [details]
> Patch
> 
> Not asking for review, just saving the work in progress...
> 
> Thanks Mark for you comment on Bug 131082, it seems I missed the obvious...
> Calling JSContextGroupSetExecutionTimeLimit() from a WebExtension / injected
> bundle is working fine... while using the appropriate callback.
> 
> In this concern "load_started" is fine because it is calling
> JSContextGroupSetExecutionTimeLimit() for each page load hence reseting
> m_didFire state
> 
> I can try to use WTF::WorkQueue, it sounds great!
> Actually it seems we could apply the same treatment to remove / refactor
> heap/HeapTimer.cpp/.h

FYI, since I didn’t hear back from you for a bit, I’ve actually implemented the changes to use WorkQueue (I think).  I’m currently testing it for some edge cases.  I plan upload a patch soon.  Thought I’d let you know so that we don’t do redundant work.

[Mark Lam]

*** Bug 147473 has been marked as a duplicate of this bug. ***

[Mark Lam]

Created attachment 258110 [details]
the fix: using WorkQueue.

I'll test this on at least one other platform first before marking it for review.

[Mark Lam]

Created attachment 258128 [details]
fix 2: Don't #include "Watchdog.h" in VM.h.  No more compiling the world.

[Mark Lam]

Need some EFL and GTK help.

EFL is failing to build due its implementation of WorkQueue:
https://webkit-queues.webkit.org/results/12528

GTK is also failing to build due to its implementation of WorkQueue:
https://webkit-queues.webkit.org/results/12622

[Mark Lam]

Created attachment 258137 [details]
fix 3: fixed build + some bug fixes.

[Michael Catanzaro]

Unbelievable how slow the GTK EWS is during Spanish nighttime. :(

Anyway the previous include directory issue on the EWS is concerning but I can't reproduce it locally; I built with attachment #258137 [details] here with absolutely no issues. So I was then waiting for the EWS to provide more feedback on the new attachment, but I guess I will check back tomorrow as it still isn't done yet. Anyway the original problem for GTK was:

FAILED: /usr/lib/ccache/c++   -DBUILDING_GTK__=1 -DBUILDING_WITH_CMAKE=1 -DDATA_DIR=\"share\" -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1 -DSTATICALLY_LINKED_WITH_WTF -DUSER_AGENT_GTK_MAJOR_VERSION=602 -DUSER_AGENT_GTK_MINOR_VERSION=1 -DWEBKITGTK_API_VERSION_STRING=\"4.0\" -std=c++11 -O3 -DNDEBUG -fno-exceptions -fno-strict-aliasing -fno-rtti -I../../Source/JavaScriptCore/shell/. -I. -I../../Source/JavaScriptCore -I../../Source/JavaScriptCore/API -I../../Source/JavaScriptCore/ForwardingHeaders -I../../Source/JavaScriptCore/assembler -I../../Source/JavaScriptCore/bindings -I../../Source/JavaScriptCore/builtins -I../../Source/JavaScriptCore/bytecode -I../../Source/JavaScriptCore/bytecompiler -I../../Source/JavaScriptCore/dfg -I../../Source/JavaScriptCore/disassembler -I../../Source/JavaScriptCore/ftl -I../../Source/JavaScriptCore/heap -I../../Source/JavaScriptCore/debugger -I../../Source/JavaScriptCore/inspector -I../../Source/JavaScriptCore/inspector/agents -I../../Source/JavaScriptCore/inspector/augmentable -I../../Source/JavaScriptCore/inspector/remote -I../../Source/JavaScriptCore/interpreter -I../../Source/JavaScriptCore/jit -I../../Source/JavaScriptCore/llint -I../../Source/JavaScriptCore/llvm -I../../Source/JavaScriptCore/parser -I../../Source/JavaScriptCore/profiler -I../../Source/JavaScriptCore/replay -I../../Source/JavaScriptCore/runtime -I../../Source/JavaScriptCore/tools -I../../Source/JavaScriptCore/wasm -I../../Source/JavaScriptCore/yarr -I../../Source/WTF -IDerivedSources -IDerivedSources/ForwardingHeaders -IDerivedSources/JavaScriptCore -IDerivedSources/JavaScriptCore/inspector -I../../Source -I../../Source/JavaScriptCore/disassembler/udis86 -MMD -MT Source/JavaScriptCore/shell/CMakeFiles/jsc.dir/__/jsc.cpp.o -MF Source/JavaScriptCore/shell/CMakeFiles/jsc.dir/__/jsc.cpp.o.d -o Source/JavaScriptCore/shell/CMakeFiles/jsc.dir/__/jsc.cpp.o -c ../../Source/JavaScriptCore/jsc.cpp
In file included from ../../Source/WTF/wtf/WorkQueue.h:42:0,
                 from ../../Source/JavaScriptCore/runtime/Watchdog.h:32,
                 from ../../Source/JavaScriptCore/runtime/VM.h:53,
                 from ../../Source/JavaScriptCore/heap/CopiedSpaceInlines.h:32,
                 from ../../Source/JavaScriptCore/runtime/ButterflyInlines.h:31,
                 from ../../Source/JavaScriptCore/runtime/JSArray.h:25,
                 from ../../Source/JavaScriptCore/runtime/ArrayPrototype.h:24,
                 from ../../Source/JavaScriptCore/jsc.cpp:25:
../../Source/WTF/wtf/glib/GMainLoopSource.h:32:18: fatal error: glib.h: No such file or directory
 #include <glib.h>
                  ^
compilation terminated.

The glib include directory is clearly missing from the build command in this EWS failure, but I'm not sure why, because it's very clearly specified in JSC's PlatformGTK.cmake. You shouldn't need any further changes to CMakeLists.txt. I half want to say "whatever works for me must be good" but I'm also half concerned that the GTK and EWS bots have very similar include directory issues....

[Mark Lam]

Created attachment 258145 [details]
fix 4: enable watchdog tests for all platforms.

[Mark Lam]

Created attachment 258150 [details]
fix 5: more build fix for Windows port.

[Gyuyoung Kim]

(In reply to comment #25)
> Created attachment 258150 [details]
> fix 5: more build fix for Windows port.


Mark, please add a include path for DispatchQueueEfl.h to PlatformEfl.cmake in order to pass efl-ews.


diff --git a/Source/JavaScriptCore/PlatformEfl.cmake b/Source/JavaScriptCore/PlatformEfl.cmake
index 6cc8c0e..ce1bc0b 100644
--- a/Source/JavaScriptCore/PlatformEfl.cmake
+++ b/Source/JavaScriptCore/PlatformEfl.cmake
@@ -2,6 +2,7 @@ list(APPEND JavaScriptCore_INCLUDE_DIRECTORIES
     ${ECORE_INCLUDE_DIRS}
     ${EINA_INCLUDE_DIRS}
     ${EO_INCLUDE_DIRS}
+    "${WTF_DIR}/wtf/efl"
 )
 add_definitions(-DSTATICALLY_LINKED_WITH_WTF)
 
diff --git a/Source/WebCore/PlatformEfl.cmake b/Source/WebCore/PlatformEfl.cmake
index c1f6f4c..b4bb2d8 100644
--- a/Source/WebCore/PlatformEfl.cmake
+++ b/Source/WebCore/PlatformEfl.cmake
@@ -24,6 +24,7 @@ list(APPEND WebCore_INCLUDE_DIRECTORIES
     "${WEBCORE_DIR}/platform/network/soup"
     "${WEBCORE_DIR}/platform/text/efl"
     "${WEBCORE_DIR}/plugins/efl"
+    "${WTF_DIR}/wtf/efl"
 )

[Mark Lam]

Created attachment 258154 [details]
fix 6: + EFL fix, and fixed a few other bugs from debug code that was accidentally left in.

[Geoffrey Garen]

Comment on attachment 258154 [details]
fix 6: + EFL fix, and fixed a few other bugs from debug code that was accidentally left in.

View in context: https://bugs.webkit.org/attachment.cgi?id=258154&action=review

As I said in https://bugs.webkit.org/show_bug.cgi?id=147473#c9, I think you should remove the timer ID concept, and exclusively use expiration time. If a wakeup happens before our expiration time, we ignore it. If a wakeup happens after our expiration time, we signal a timeout.

I also think you should remove the isArmed bit. Disarming the timer should just set its expiration time far into the future. That way, the watchdog has only one bit of state: when will I expire?

Since Watchdog accesses Watchdog::Timer and Watchdog::Timer accesses Watchdog, I think that separating them into two classes is confusing, and makes for spaghetti code. I would make them once class.

Watchdog::didFire should split between an inline fast path and a slow path. The fast path should verify that we did not fire. The slow path should reset the did fire bit and perform the other expensive computations.

> Source/JavaScriptCore/runtime/Watchdog.cpp:76
> +    auto steadyTimeSinceEpoch = std::chrono::steady_clock::now().time_since_epoch();
> +    auto currentTime = std::chrono::duration_cast<std::chrono::seconds>(steadyTimeSinceEpoch);
> +    if (m_activeTimerID) {
> +        if ((m_expirationTime > currentTime) && (m_expirationTime - currentTime <= timeout))
> +            return;
> +    }

This seems wrong in the case where the caller wants to change the time limit to be longer than it used to be. You need to update the expiration time to reflect the new, longer limit.

[Mark Lam]

(In reply to comment #28)
> Comment on attachment 258154 [details]
> fix 6: + EFL fix, and fixed a few other bugs from debug code that was
> accidentally left in.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258154&action=review
> 
> As I said in https://bugs.webkit.org/show_bug.cgi?id=147473#c9, I think you
> should remove the timer ID concept, and exclusively use expiration time. If
> a wakeup happens before our expiration time, we ignore it. If a wakeup
> happens after our expiration time, we signal a timeout.
> 
> I also think you should remove the isArmed bit. Disarming the timer should
> just set its expiration time far into the future. That way, the watchdog has
> only one bit of state: when will I expire?

Oh, I think I better understand what you’re suggesting now.  I was focusing on the part about when we actually need to start new timers, and missed the significance of doing away with the TimerIDs.  I agree that this is a better approach.  Sorry I didn’t catch on the first time.  Will fix.

I do have to point out one subtle difference from your suggestion that I will have to apply though: if the wakeup happens before our expiration time, I can’t just ignore it if the watchdog is not disarmed.  I will need to start a new timer for the balance of the time in m_limit.  This is because dispatchAfter() triggers wakeups based on wall clock / monotonic time, and definitely no CPU time.  It is, after all, triggered by a timer on the dispatched thread, not a timer on the JS thread which we’re trying to keep to a CPU time limit.  Hence, I have to keep track of more than just one time value.  This detail is probably why you didn’t get what I’m doing in Source/JavaScriptCore/runtime/Watchdog.cpp:76 below.

Based on this, I think I should also choose more descriptive names for the Watchdog fields.

> Since Watchdog accesses Watchdog::Timer and Watchdog::Timer accesses
> Watchdog, I think that separating them into two classes is confusing, and
> makes for spaghetti code. I would make them once class.

I debated about that, but opted to keep Timer to minimize code change.  But in the end, I’m not sure it was worth it anyway.  Will fix as you ask.

> Watchdog::didFire should split between an inline fast path and a slow path.
> The fast path should verify that we did not fire. The slow path should reset
> the did fire bit and perform the other expensive computations.

This completely slipped my mind.  Will fix.

> > Source/JavaScriptCore/runtime/Watchdog.cpp:76
> > +    auto steadyTimeSinceEpoch = std::chrono::steady_clock::now().time_since_epoch();
> > +    auto currentTime = std::chrono::duration_cast<std::chrono::seconds>(steadyTimeSinceEpoch);
> > +    if (m_activeTimerID) {
> > +        if ((m_expirationTime > currentTime) && (m_expirationTime - currentTime <= timeout))
> > +            return;
> > +    }
> 
> This seems wrong in the case where the caller wants to change the time limit
> to be longer than it used to be. You need to update the expiration time to
> reflect the new, longer limit.

No, this is correct.  m_expirationTime indicates when the active timer is expected to expire.  m_limit indicates the amount of CPU time we want to allow the JS code to run.  If the active timer will expire before the amount of time allowed by m_limit, we’ll just start a new timer to make up the difference.  Hence, if the active timer claims that it will expire before our estimated timeout, we just leave it be.  For well behave JS code, there’s a high probability that we’ll disarm the watchdog before the active timer expires anyway.

[Geoffrey Garen]

> I do have to point out one subtle difference from your suggestion that I
> will have to apply though: if the wakeup happens before our expiration time,
> I can’t just ignore it if the watchdog is not disarmed.  I will need to
> start a new timer for the balance of the time in m_limit.  This is because
> dispatchAfter() triggers wakeups based on wall clock / monotonic time, and
> definitely no CPU time.  It is, after all, triggered by a timer on the
> dispatched thread, not a timer on the JS thread which we’re trying to keep
> to a CPU time limit.  Hence, I have to keep track of more than just one time
> value.  This detail is probably why you didn’t get what I’m doing in
> Source/JavaScriptCore/runtime/Watchdog.cpp:76 below.

I suggest keeping a wall clock expiration time and a CPU expiration time. 

When our dispatchAfter calls us back, check against wall clock expiration, and do what I said above.

If we have hit our expected wall clock expiration, then check against CPU expiration and either (a) signal a timeout or (b) update wall clock expiration and dispatchAfter again.

[Mark Lam]

(In reply to comment #28)
> I also think you should remove the isArmed bit. Disarming the timer should
> just set its expiration time far into the future. That way, the watchdog has
> only one bit of state: when will I expire?

Regarding the isArmed state, after looking at the code, I realized that it is still needed.  Here’s why: I need to track whether VM re-entry count.  The watchdog should only be armed (where we set the expiration time) on first entry into the VM, and disarmed (where we clear the expiration time) when the re-entry count reaches 0.

If the client calls Watchdog::setTimeLimit() after we have entered the VM (i.e. the watchdog should be armed), we’ll set the expiration time immediately.  An example of this is during the watchdog timeout callback to the client, where the client may set a new watchdog time limit or reuse the existing one.

If the client calls Watchdog::setTimeLimit() before entering the VM (i.e. the watchdog is not armed yet), then we need to defer computing the expiration time until we actually enter the VM (when we arm the watchdog).

[Geoffrey Garen]

> Regarding the isArmed state, after looking at the code, I realized that it
> is still needed.  Here’s why: I need to track whether VM re-entry count. 
> The watchdog should only be armed (where we set the expiration time) on
> first entry into the VM, and disarmed (where we clear the expiration time)
> when the re-entry count reaches 0.

This doesn't sound like an isArmed state.

> If the client calls Watchdog::setTimeLimit() after we have entered the VM
> (i.e. the watchdog should be armed), we’ll set the expiration time
> immediately.  An example of this is during the watchdog timeout callback to
> the client, where the client may set a new watchdog time limit or reuse the
> existing one.

This sounds like an update to the wall clock deadline.

> If the client calls Watchdog::setTimeLimit() before entering the VM (i.e.
> the watchdog is not armed yet), then we need to defer computing the
> expiration time until we actually enter the VM (when we arm the watchdog).

This sounds like a time limit data member.

So, setTimeLimit should update a time limit data member, check if JavaScript is currently executing, and if so, start a timer.

[Mark Lam]

(In reply to comment #32)
> > Regarding the isArmed state, after looking at the code, I realized that it
> > is still needed.  Here’s why: I need to track whether VM re-entry count. 
> > The watchdog should only be armed (where we set the expiration time) on
> > first entry into the VM, and disarmed (where we clear the expiration time)
> > when the re-entry count reaches 0.
> 
> This doesn't sound like an isArmed state.

Sounds like your beef is with the use of the term “arm”, which I agree is not the best choice.  So, let’s clarify: the watchdog should only be started if both of the following conditions (or “states” as I used to call them) are true:

1. the user has set a time limit.
   I current called this state “enabled”.
   I will rename this state to “hasTimeLimit”.

2. execution has entered the VM.
   I currently called this state “armed”, mostly because “enabled” was already in use, and I was grasping for another term which sort of means the same thing.  The idea of “arm” came from the fact that we do this every time we enter the VM.  Hence, at VM entry (or re-entry), we tell the watchdog to arm itself (with the understood condition that it will only do so if it is “enabled”).  I agree that this may not be the best choice.
   I will renamed this state to “hasEnteredVM”.

I presume you’ll be happier with these terms: “hasTimeLimit” and “hasEnteredVM”?

> > If the client calls Watchdog::setTimeLimit() after we have entered the VM
> > (i.e. the watchdog should be armed), we’ll set the expiration time
> > immediately.  An example of this is during the watchdog timeout callback to
> > the client, where the client may set a new watchdog time limit or reuse the
> > existing one.
> 
> This sounds like an update to the wall clock deadline.

Actually, it’s the CPU clock deadline that I need to update, and yes, I’m already doing that.

> > If the client calls Watchdog::setTimeLimit() before entering the VM (i.e.
> > the watchdog is not armed yet), then we need to defer computing the
> > expiration time until we actually enter the VM (when we arm the watchdog).
> 
> This sounds like a time limit data member.
> 
> So, setTimeLimit should update a time limit data member, check if JavaScript
> is currently executing, and if so, start a timer.

Yes, I am already doing this.  I check for whether “JavaScript is currently executing” by checking my previously named “armed” state.  I will now check that “hasEnteredVM” is true.

[Mark Lam]

Created attachment 258629 [details]
fix 7: no more timerID, no more ARMed state, no more WatchdogScope.

[WebKit Commit Bot]

Attachment 258629 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:32:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/runtime/Watchdog.cpp:35:  Watchdog::NO_TIMEOUT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/Watchdog.cpp:147:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/Watchdog.cpp:162:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/Watchdog.h:72:  NO_TIMEOUT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/Watchdog.h:102:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 258629 [details]
fix 7: no more timerID, no more ARMed state, no more WatchdogScope.

View in context: https://bugs.webkit.org/attachment.cgi?id=258629&action=review

> Source/JavaScriptCore/ChangeLog:32
> +		   If m_cpuTimeDeadline has been reached, the watchdog is considered to have fired.

Tab in whitespace.  Will fix before landing.

[Mark Lam]

Created attachment 258632 [details]
fix 8: fix style issues.

There a few remaining style complaints about the use of NO_TIMEOUT as a constant, and the space in std::function<void ()>.  These are intentional because we do use capital letters and underscores for constants, and the std::function<void ()> is a pre-existing pattern in WorkQueue.h.

[WebKit Commit Bot]

Attachment 258632 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Watchdog.cpp:35:  Watchdog::NO_TIMEOUT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/Watchdog.h:72:  NO_TIMEOUT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/Watchdog.h:102:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 258632 [details]
fix 8: fix style issues.

View in context: https://bugs.webkit.org/attachment.cgi?id=258632&action=review

r=me

> Source/JavaScriptCore/ChangeLog:19
> +           Why do we need 2 time deadlines: m_cpuTimeDeadline and m_currentMonotonicTimeDeadline?

Let's call this m_cpuDeadline and m_wallClockDeadline.

Deadline implies time, so we don't need time in the name.

Monotonic only means "increasing", so it does not explain its difference from CPU, which is also an increasing measurement of time.

> Source/JavaScriptCore/ChangeLog:45
> +           timer.  All other wake ups is considered to be spurious and will be ignored.

s/is/are/

> Source/JavaScriptCore/API/JSContextRef.cpp:125
> +        vm.watchdog->setTimeLimit(vm, Watchdog::NO_TIMEOUT);

WebKit style is "noTimeout".

The function here is "set time limit". Its input should be a "time limit". So, let's name the constant "noTimeLimit".

> Source/JavaScriptCore/runtime/Watchdog.cpp:37
> +static std::chrono::microseconds currentMonotonicTime()

Let's call this currentWallClockTime.

> Source/JavaScriptCore/runtime/Watchdog.cpp:46
> +    , m_timeoutPeriod(NO_TIMEOUT)

Since this is set by setTimeLimit, and compared to noTimeLimit, let's call it m_timeLimit.

> Source/JavaScriptCore/runtime/Watchdog.cpp:55
> +    auto watchdog = this;
> +    m_timerHandler = [watchdog] {

You can just capture [this] instead.

[Mark Lam]

Thanks for the review.

(In reply to comment #39)
> > Source/JavaScriptCore/ChangeLog:19
> > +           Why do we need 2 time deadlines: m_cpuTimeDeadline and m_currentMonotonicTimeDeadline?
> 
> Let's call this m_cpuDeadline and m_wallClockDeadline.
> 
> Deadline implies time, so we don't need time in the name.
> 
> Monotonic only means "increasing", so it does not explain its difference
> from CPU, which is also an increasing measurement of time.

Actually, I did mean increasing time.  When I think of wall clock time, I think of the clock on the wall that a user can modify (or day light savings time can kick in and change) which potentially interferes with my measurements.  Instead, I want the monotonically increasing clock time.  The std::chrono spec (http://en.cppreference.com/w/cpp/chrono) also uses the terms wall clock time (std:chrono::system_clock) and monotonic time (std:chrono::steady_clock) similarly.

But for our purpose here, wall clock time conveys the contrast from CPU time well enough.  I’ll change the names as you suggest.

I’ll also make all the other fixes you requested, and fix up variable names and comments to be consistent with the change.

[Mark Lam]

Created attachment 258806 [details]
patch for landing.

[Mark Lam]

Created attachment 258807 [details]
patch for landing v2: fix up some comments.

[Mark Lam]

Landed in r188329: <http://trac.webkit.org/r188329>.

[Geoffrey Garen]

(In reply to comment #40)
> When I think of wall clock time, I think of the clock on the wall that a user can modify

In CS, "wall clock time" is a term of art for real time in contrast to CPU time: <https://en.wikipedia.org/wiki/Wall-clock_time>.