refactor and modernize the native interface #35

xqp · 2017-09-12T12:21:09Z

as discussed yesterday with @maxkorp in #34 here are a bunch of changes.

remove openpa dependencies
new build enviroment for libnsfw (CMakeLists)
use modern c++ structures and classes (unique_ptr and concurrency wrapper)
modernize the native platform implementation with RAII wrapper
replace Lock.cpp with std::unique_lock<mutex_type>

to be done in future work:
~~- implement a c++ callback interface to the native filewatchers~~ (in other PR)

Make it work on macOS

FIX: race condition due to condition variable misuse

Remove OpenPA

reneme · 2017-09-12T15:06:43Z

Open build and threading issues:

~~race condition on windows~~ (seemed to have been a problem in VC++ 2013 -.-)
~~macOS build fails with node.js v4 (not sure why, input would be welcome)~~
(needed to set 'MACOSX_DEPLOYMENT_TARGET': '10.7' in xcode_settings)

implausible · 2017-09-12T21:47:09Z

Ahhh. This would be greatto get rid of opa! Will look into your issues with macos. I need to start acquiring more time at work for this project. :)

implausible · 2017-09-13T06:37:07Z

binding.gyp

+                "xcode_settings": {
+                    "OTHER_CFLAGS": [
+                        "-std=c++0x",
+                        "-stdlib=libc++"


I think we can update this to:

"-std=c++11"

Shouldn't need to define stdlib=c++

FIX: build with node.js v4 on macOS

reneme · 2017-09-13T11:17:47Z

We are seeing several races and dead locks both on Travis and AppVeyor. Frankly we believe that they've been in the code base before (see build results of latest master merge). It might be that implementing EventQueue based on C++11 constructs rather than OpenPA is encouraging these to show up. Did you see flaky tests or behaviour of watcher startup in the past?

…TL' into refactor/remove_opa_and_use_cpp11_STL

Refactor/remove opa and use cpp11 stl

Refactor: use std::thread rather then pthread_t

…1_STL use semaphore to wait until thread is running

implausible · 2017-09-14T18:49:26Z

The polling system actually needs to be removed all together with a throttled callback mechanism.

I'm thinking of using something similar in pattern to what I'm doing in https://github.com/implausible/find-git-repositories/blob/master/src/FindGitRepos.cpp#L134.

Honestly, the the synchronization behavior of the polling thread is not my proudest set up.

reneme · 2017-09-15T06:47:31Z

The polling system actually needs to be removed all together with a throttled callback mechanism.

Agreed. That sounds a lot better than another spinning thread polling the EventQueue. A similar approach could be useful for our envisioned native API to the native watchers as well.

reneme · 2017-09-15T07:46:31Z

From our side, this PR is complete and we'd be happy for feedback. Frankly this became a much bigger patch than we initially thought. There are a few more things to be done from our perspective, but I suggest to open another issue for that.

Summary of changes:

add a basic build system based on CMake (CMakeLists.txt and src/CMakeLists.txt). CMake is widely used in the C++ world and hence is very convenient for integrating with C++ projects. Down the road we should see how to consolidate this with binding.gyp I guess.
remove the dependency on openPA that was used to implement EventQueue and replace it with std::deque and a std::mutex. This is much simpler to use and should serve the purpose just fine. Furthermore it appeared to me that OpenPA is actually not developed further anymore. (no commits since 2014 on their repo)
switch to a C++11 compatible compiler across all platforms (that proved to be a bit tricky on macOS and node.js v4: (see here).
remove the scoped lock implementation in class Lock and replace it with std::lock_guard and std::unique_lock respectively.
add the class SingletonSemaphore (see docstrings for details) as a convenience wrapper around std::condition_variable. We use this to signal the successful worker thread startup on all platforms (replacing uv_sem_t on the macOS implementation).
replace pthread_mutex_t and pthread_t with std::mutex and std::thread where possible. On the linux watcher we left pthread_t, because std::thread doesn't allow to cancel a running thread. (which IMHO isn't a good thing to do anyway -> future work)
Modernize and refactor the implementation of the windows watcher implementation. We introduced a Controller class that is responsible to provide the API used in NativeInterface and handles the directory to be watched. Furthermore there is a Watcher that is responsible for the worker thread handling, the ReadDirectoryChangesW call and the event interpretation.
Fixed some races, potential dead locks and memory leaks along the way

implausible · 2017-09-18T06:03:11Z

I haven't had a chance to sit down and review this seriously; however, the only immediate concern I have is the use of std::deque with std::mutex. The reason for OpenPA was the lock free queue that came with it (that also worked cross platform and prior to c++11), was so that insertions from the file watchers and the callback consumer could run simultaneously without strong synchronization. With the use of std::deque and the std::mutex, has there been any work to address that difference? IE removing items from the queue before sending them to the callback on the polling thread? For instance, we wouldn't want to add any additional locking on the main thread in javascript where the callback reads from the queue.

reneme · 2017-09-18T14:23:45Z

Thanks for your quick reply! Regarding your concerns:

With the use of std::deque and the std::mutex, has there been any work to address that difference?

No. In fact the EventQueue now locks a mutex on EventQueue::dequeueAll() (see here).

The reason for OpenPA was the lock free queue that came with it

Did you measure any lock contention on a mutex-based approach before? While I agree that lock free data structures are fancy, I wouldn't go down that road without evident benefits.

For instance, we wouldn't want to add any additional locking on the main thread in javascript where the callback reads from the queue.

I guess you are afraid to starve the javascript execution because of a quickly spinning file event watcher. This seems reasonable but I would argue that it is very unlikely. The EventQueue mutex is acquired by the watchers while posting new events only. They even unlock the mutex after each individual enqueue() (see here).

Both Windows and Linux use blocking and/or asynchronous calls (ReadDirectoryChangesW() and read()) in their watcher implementations. Frankly I'm not sure about the macOS implementation but I would be surprised if its any different. Hence, those threads will yield the CPU at latest after each file event cycle.

Anyhow, I agree that it would be interesting to benchmark this.

reneme · 2017-09-18T20:52:53Z

Following up on that with some quick'n'dirty measuring attempt using std::mutex::try_lock() in EventQueue::dequeueAll() to approximate (documentation says, that try_lock() is allowed to falsely return false) the contention on my MacBook Pro.

I ran a minimal C++ program that would frequently retrieve all file events from the EventQueue in a separate thread just like the NSFW adapter would. The polling thread was put to sleep after each poll for 1, 5, 10, 25, 100, 250 and 500ms. Each of the frequencies were used for 20 seconds, checking how often try_lock() would report the lock in EventQueue::dequeueAll() to be already held. Concurrently, I ran this script to rapidly create 1,000,000 tiny files in the watched directory:

for i in $(seq 1 1000); do
  mkdir -p $i
  for j in $(seq 1 1000); do
    echo "foobar" > $i/$j
  done
done

The graph below shows how often EventQueue::dequeueAll() wasn't able to immediately acquire the lock per second. In which case I would fall back to a standard std::mutex::lock() (effectively blocking the polling thread). Furthermore I measured the time until the lock() call returned using std::chrono::high_resolution_clock which averaged at less than six microseconds.

That means for a reasonable query frequency of say 100ms, the polling thread would only experience around one blocking std::mutex::lock() invocation every 10s holding it back for around 6µs. Notably with a quite unusual file operation workload of more than 6,400 file/directory creations per second.

Edit: The same test on windows and linux show comparable results.

implausible · 2017-09-20T01:29:03Z

That should be sufficient. Thank you for providing some numbers.

implausible · 2017-09-20T01:30:15Z

appveyor.yml

@@ -15,7 +15,7 @@ install:
  # Get the latest stable version of Node.js or io.js
  - ps: Install-Product node $env:nodejs_version
  # install modules
-  - npm install --msvs_version=2013
+  - npm install --msvs_version=2015


Will this still build with msvs_version set to 2013?

let us use the 2013 compiler :)

implausible

This is a first pass. I just stopped at the windows controller/watcher refactor. Will pick up again tomorrow. Most of what I've noticed today is just trying to keep the voice of the project consistent. It's looking great. Will have more on the refactor soon.

implausible · 2017-09-20T01:41:09Z

includes/SingleshotSemaphore.h

+     * Blocks the calling thread until the semaphore is signaled asynchronously.
+     * If `signal()` has been called on the semaphore already, this won't block.
+     */
+    void wait()


The rest of the project is using 2 spaces for indent and opening braces on the function definition (generally). We should perhaps invest some time in declaring a public style guide now that we are receiving additional contributions, but in general we should try to adhere to the style already used in the base.

maybe we should use the llvm styleguide
https://llvm.org/docs/CodingStandards.html

implausible · 2017-09-20T01:49:13Z

includes/NativeInterface.h


  std::string getError();
-  std::vector<Event *> *getEvents();
+  std::vector<Event *>* getEvents();


We're going to need to pick a style, especially in this PR you're swapping between voices. Pointer/reference operator should live on the variable name since comma that is what the operator affects. IE:

int* a, b;

in the above, only a is an integer pointer, where as it seems to be implied that both a and b are integer pointers. I will add a style comment anywhere that I've noticed that.

implausible · 2017-09-20T01:50:01Z

includes/Queue.h

-    std::string directory,
-    std::string fileA,
-    std::string fileB = ""
+    const std::string& directory,


style please :)

implausible · 2017-09-20T01:51:21Z

includes/SingleshotSemaphore.h

+    }
+
+  private:
+    std::mutex              mMutex;


We aren't enforcing this variable alignment in the code base. Single space will suffice. Thanks!

implausible · 2017-09-20T02:02:49Z

src/NSFW.cpp

  if (mNSFW->mInterface == NULL) {
    uv_mutex_unlock(&mNSFW->mInterfaceLock);
    return;
  }
+  uv_mutex_unlock(&mNSFW->mInterfaceLock);


the lock manage here is really difficult. I think we can do better, but for another PR. Just to talk about what I'm currently mulling over...

We should consider building a mapping interface between the native watcher and the javascript head. When an NSFW is created on the js side, it should assign itself the next id and create a native file watcher. The native file watcher should know the id or have a callback which knows of the id and be able to reference the js side through that id (when performing callbacks). When the nsfw module itself halts, we should let the native side clean up on its own time, but sever the map between the native and js layer... That way we don't need to sync at all in the stop worker!

What do you think? Just thoughts for another PR I think.

implausible · 2017-09-20T02:04:31Z

src/Queue.cpp


-    Event *event = node->event;
-    delete node;
+  auto& front = queue.front();


Style please.

implausible · 2017-09-20T02:04:52Z

src/NativeInterface.cpp

-  }
-
-  return events;
+std::vector<Event *>* NativeInterface::getEvents() {


style please.

implausible · 2017-09-20T02:05:32Z

src/Queue.cpp

+  const auto queueSize = queue.size();
+  std::unique_ptr<std::vector<Event*>> events(new std::vector<Event*>(queueSize, nullptr));
+  for (size_t i = 0; i < queueSize; ++i) {
+      auto& front = queue.front();


style please.

implausible · 2017-09-20T02:05:48Z

src/Queue.cpp


-  OPA_Queue_enqueue(&mQueue, node, EventNode, header);
-  OPA_incr_int(&mNumEvents);
+void EventQueue::enqueue(const EventType type, const std::string& directory, const std::string& fileA, const std::string& fileB) {


implausible · 2017-09-20T02:08:20Z

src/osx/RunLoop.cpp

-    scheduleRunLoopWork,
-    (void *)this
-  );
+  mRunLoopThread = std::thread([] (RunLoop *rl) { rl->work(); }, this);


Ahhh, lambdas.

…b.com/xqp/nsfw into refactor/remove_opa_and_use_cpp11_STL

xqp · 2017-09-20T08:54:22Z

About style issues:
we should use the google or llvm style guide and a formater like "clang-format" to format our code automatically.

xqp · 2017-09-20T19:50:24Z

we should use the new msvc 15 because the older version can not compile the actual code version.
with msvc 2015 this compile unit works fine

implausible

Mostly style and some areas we could improve clarity, but this is looking very solid. Thanks!

implausible · 2017-09-21T23:14:56Z

src/win32/Controller.cpp

@@ -0,0 +1,73 @@
+#include "../includes/win32/Controller.h"
+
+static std::wstring convertMultiByteToWideChar(const std::string &multiByte)


Code base is currently using curly brace on the opening line.

implausible · 2017-09-21T23:16:54Z

src/win32/Controller.cpp

+}
+
+HANDLE Controller::openDirectory(const std::wstring &path)
+{


implausible · 2017-09-21T23:18:57Z

src/win32/Controller.cpp

+}
+
+Controller::~Controller()
+{


Open on same line, please

implausible · 2017-09-21T23:19:46Z

src/win32/Controller.cpp

+}
+
+std::string Controller::getError()
+{


Same line here

implausible · 2017-09-21T23:19:53Z

src/win32/Controller.cpp

+  return mWatcher->getError();
+}
+
+bool Controller::hasErrored()


implausible · 2017-09-21T23:43:19Z

src/win32/Watcher.cpp

+void Watcher::stop()
+{
+  mRunning = false;
+  QueueUserAPC([](__in ULONG_PTR) {}, mRunner.native_handle(), (ULONG_PTR)this);


Can we add a comment here, it would be a lot faster for future contributors to recognize that this is to wake up the SleepEx above so that the watcher's loop exits cleanly.

implausible · 2017-09-21T23:44:27Z

src/win32/Watcher.cpp

+  }
+}
+
+void Watcher::stop()


{ on the same line please

implausible · 2017-09-21T23:44:39Z

src/win32/Watcher.cpp

+    return;
+  }
+
+  QueueUserAPC([](__in ULONG_PTR self) {


life is so much better with lambdas.

implausible · 2017-09-21T23:44:50Z

src/win32/Watcher.cpp

+  mRunner.join();
+}
+
+void Watcher::setError(const std::string &error)


{ on the same line please

implausible · 2017-09-21T23:44:58Z

src/win32/Watcher.cpp

+}
+
+
+std::string Watcher::getError() const


{ on the same line please

FIX: review comments

FIX: missing header adaption

implausible · 2017-09-22T15:03:25Z

We had a test failure on linux in the test for listening for the creation of a deeply nested filex; however, I have seen that failure in master. I don't think it's related to anything you've done, since the refactor there is minimal and should have no change in semantics.

I've gone ahead and restarted that, test. Though we should open an issue related to discovering why that test fails.

Patrik Fiedler and others added 11 commits September 12, 2017 11:07

refactor the queue implementation

92e1fd5

FIX: remove dependency to uv_sem_t in osx service

5ec8b6e

extend CMakeLists.txt for platform dependent code

4425157

FIX: use const& of std::string

62939cd

Merge pull request #1 from reneme/refactor/remove_opa_and_use_cpp11_STL

b36a028

Make it work on macOS

FIX: usage of condition variable

3015c9d

use the javascript tests with windows

5fc4dbd

Merge pull request #2 from reneme/refactor/remove_opa_and_use_cpp11_STL

2137636

FIX: race condition due to condition variable misuse

remove dependency to OpenPA

87b2e59

FIX: signed/unsigned compiler warning

28701c9

Merge pull request #3 from reneme/refactor/remove_opa_and_use_cpp11_STL

e91446e

Remove OpenPA

xqp changed the title ~~refactor and modernize the native interafe~~ refactor and modernize the native interface Sep 12, 2017

Patrik Fiedler added 3 commits September 12, 2017 15:09

change xcode configuration for nodejs 4 and 6

5d11a18

try to find a reace cond

5c7fe7d

try msvs 2015

feb3b0e

implausible reviewed Sep 13, 2017

View reviewed changes

reneme and others added 6 commits September 13, 2017 09:35

FIX: build on macOS with node.js v4

e7a94c3

switch from c++0x to proper c++11

bfe9eee

Refactor: add a single shot semaphore for threadsafe state announcement

4978e9a

FIX: remove unnecessary implementation

747d16e

Merge pull request #4 from reneme/refactor/remove_opa_and_use_cpp11_STL

9c58e58

FIX: build with node.js v4 on macOS

merge with origin/master

438cf33

reneme and others added 5 commits September 13, 2017 13:42

FIX: code cosmetics

ed56d42

Merge remote-tracking branch 'xqp/refactor/remove_opa_and_use_cpp11_S…

a65bca5

…TL' into refactor/remove_opa_and_use_cpp11_STL

refactor the windows native implementation

1e667d0

implement error handling in watcher and controller

422d46b

use c++11 threads instead of native implementation

def8e5a

xqp and others added 5 commits September 14, 2017 18:24

Merge pull request #6 from reneme/refactor/remove_opa_and_use_cpp11_STL

60ade73

Refactor/remove opa and use cpp11 stl

Refactor: use std::thread rather then pthread_t

27d1677

Merge pull request #7 from reneme/refactor/remove_opa_and_use_cpp11_STL

da6e002

Refactor: use std::thread rather then pthread_t

use semaphore to wait until thread is running

0a8befa

Merge pull request #8 from hrantzsch/refactor/remove_opa_and_use_cpp1…

f224dbf

…1_STL use semaphore to wait until thread is running

implausible reviewed Sep 20, 2017

View reviewed changes

implausible suggested changes Sep 20, 2017

View reviewed changes

Patrik Fiedler added 2 commits September 20, 2017 10:47

review issues

c159680

Merge branch 'refactor/remove_opa_and_use_cpp11_STL' of https://githu…

6daa85f

…b.com/xqp/nsfw into refactor/remove_opa_and_use_cpp11_STL

style guide issue

7cd1053

upgrade to msvc 2015 because compile error in older version

2674f64

implausible suggested changes Sep 21, 2017

View reviewed changes

reneme and others added 6 commits September 22, 2017 09:30

FIX: review comments

b782c47

Merge pull request #9 from reneme/refactor/remove_opa_and_use_cpp11_STL

b651b28

FIX: review comments

FIX: missing header adaption

f3a859c

Merge pull request #10 from reneme/refactor/remove_opa_and_use_cpp11_STL

a9aa805

FIX: missing header adaption

fix compile errors in windows ci build

01f1a75

remove empty line in cmakelists

990cafb

implausible approved these changes Sep 23, 2017

View reviewed changes

implausible merged commit 67771cf into Axosoft:master Sep 23, 2017

		@@ -0,0 +1,73 @@
		#include "../includes/win32/Controller.h"

		static std::wstring convertMultiByteToWideChar(const std::string &multiByte)

refactor and modernize the native interface #35

refactor and modernize the native interface #35

Conversation

xqp commented Sep 12, 2017 • edited Loading

reneme commented Sep 12, 2017 • edited Loading

implausible commented Sep 12, 2017

Choose a reason for hiding this comment

reneme commented Sep 13, 2017

implausible commented Sep 14, 2017

reneme commented Sep 15, 2017

reneme commented Sep 15, 2017

implausible commented Sep 18, 2017 • edited Loading

reneme commented Sep 18, 2017

reneme commented Sep 18, 2017 • edited Loading

implausible commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

implausible left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xqp commented Sep 20, 2017

xqp commented Sep 20, 2017

implausible left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

implausible commented Sep 22, 2017

xqp commented Sep 12, 2017 •

edited

Loading

reneme commented Sep 12, 2017 •

edited

Loading

implausible commented Sep 18, 2017 •

edited

Loading

reneme commented Sep 18, 2017 •

edited

Loading