New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parallelize main loop #239

Merged
merged 4 commits into from Mar 18, 2018

Conversation

Projects
None yet
3 participants
@JamesReynolds

JamesReynolds commented Mar 7, 2018

Fixes issue #3 and #36

The workers.py file creates:

  1. a locked_directory context manager that ensures that each thread uses a separate working directory or blocks waiting for the directory to become free.
  2. a WorkThread class that runs a job (function and args) but sets workdir for the job to a temporary folder
  3. a Workers class that manages a pool of workers using a queue to pass jobs to them.

Using this gcovr.py has been modified to run gcov inside a locked_directory context and, when complete, will move all of the resulting gcov files to the thread's working directory. This keeps the locked section short (hopefully).

main.py has been modified to use the pool Workers.

@codecov

This comment has been minimized.

codecov bot commented Mar 7, 2018

Codecov Report

Merging #239 into master will increase coverage by 1.36%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   87.35%   88.71%   +1.36%     
==========================================
  Files          12       13       +1     
  Lines        1336     1471     +135     
  Branches      243      267      +24     
==========================================
+ Hits         1167     1305     +138     
+ Misses        115      108       -7     
- Partials       54       58       +4
Impacted Files Coverage Δ
gcovr/tests/test_gcov_parser.py 100% <100%> (ø) ⬆️
gcovr/gcov.py 83.51% <88.88%> (+1.54%) ⬆️
gcovr/__main__.py 88.96% <96%> (+0.9%) ⬆️
gcovr/workers.py 98.85% <98.85%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f997eda...288fae1. Read the comment docs.

@latk

This comment has been minimized.

Member

latk commented Mar 7, 2018

Thank you for this awesome feature!

I've skimmed over the PR, and there are lots of unrelated changes regarding Docker and the README. Can we rewrite the history to exclude these changes? I'm not quite sure how to do this, possibly with git rebase --interactive or git filter-branch. Would you like to try? Otherwise, I'll be glad to help (through this PR, I have write access to your branch). While rebasing, we can also fold fixup commits (like Python 3 modifications) into another commit, as if you had always written correct code on first try.

My goal regarding the git history is that the logical evolution of this code is evident when debugging in the future, not necessarily that it records your actual process.

This PR will conflict with #237, which changes argument parsing to the argparse module. This will change how optional arguments are handled. I think your parallel_callback function can then be replaced with a nargs='?' argument.

After these points are addressed I'll take another look. E.g. we might be able to avoid some competing gcov processes by grouping work items for the same directory? And it might be possible to avoid spinning up more threads than needed? I don't know yet. But that doesn't have to be part of this PR.

Please let me know if there's anything I can do to help, especially regarding Git history simplification.

@JamesReynolds

This comment has been minimized.

JamesReynolds commented Mar 7, 2018

@latk After some experimentation I can compress this into a single commit without Dockerfile, .dockerignore or changes to README.rst using git rebase. That probably makes most sense as then my local figuring stuff out commits don't pollute anything?

This will conflict with #237 for sure. What is the best route though? Shall I wait until #237 is added then rebase?

I think we can certainly do better with competing gcov processes for certain build types. Maybe calculating the potential wd in one pass, then running the threads in a second pass selecting items that don't conflict? Another option, but only applicable to later GCC versions, is for two gcov processes to share a directory and get one to use '-x' to append hashes to its output.

@latk

This comment has been minimized.

Member

latk commented Mar 7, 2018

Ok, squashing all commits into one still results in a large commit, but the changes excluding the tests are fairly compact:

$ git diff --stat master..pr/239 gcovr/*.py
3 files changed, 207 insertions(+), 23 deletions(-)

So 👍, go ahead with squashing. Please make sure to keep relevant info from intermediate commit messages (you can edit a commit message via git commit --amend). I'll then do a proper code review in the evening or tomorrow.

I don't know whether this or #237 will be merged first, so don't rebase prematurely. Second to merge gets to resolve the conflicts :)

I don't think gcov -x helps here because it hashes the filename, but doesn't generate unique filenames. Two gcov processes that generate coverage for the same file would still clash. But yes, I was also thinking about a two-pass approach.

@JamesReynolds JamesReynolds force-pushed the JamesReynolds:upstream branch from 6afeae6 to f46c5da Mar 7, 2018

@JamesReynolds

This comment has been minimized.

JamesReynolds commented Mar 7, 2018

OK, done. I've merged all of the relevant commits into one.

With gcov -x I was thinking that half of the threads could use it and the other half not - so those two sets wouldn't conflict. It feels a bit hacky I'll admit, but it would mean you could (in some cases) double the throughput.

self.lock = Lock()
self.dirs = set()
def run_in(self, dir_):

This comment has been minimized.

@mayeut

mayeut Mar 7, 2018

Contributor

This shall probably be written using a while loop rather than using recursion.
This shall probably use a condition variable rather than just a lock to prevent the usage of time.sleep

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 8, 2018

@mayeut Fair go! I've made the change and rebased again.

This comment has been minimized.

@mayeut

mayeut Mar 8, 2018

Contributor

👍

@JamesReynolds JamesReynolds force-pushed the JamesReynolds:upstream branch 2 times, most recently from b3da876 to e1d2ba8 Mar 8, 2018

@JamesReynolds

This comment has been minimized.

JamesReynolds commented Mar 8, 2018

Rebased to #237

@JamesReynolds JamesReynolds force-pushed the JamesReynolds:upstream branch from e1d2ba8 to 58ad415 Mar 8, 2018

"""
self.cv.acquire()
self.dirs.remove(dir_)
self.cv.notify()

This comment has been minimized.

@mayeut

mayeut Mar 8, 2018

Contributor

This shall be self.cv.notify_all() instead of just self.cv.notify()

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 8, 2018

@mayeut I see why... that might explain the Windows freeze that appveyor is experiencing - because the right thread isn't being woken and we're deadlocking. Sound right?

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 8, 2018

@mayeut It certainly works now whereas it didn't before!

@JamesReynolds JamesReynolds force-pushed the JamesReynolds:upstream branch from 58ad415 to a4451e6 Mar 8, 2018

James Reynolds
Add parallel support
Fixes issues 36 and 3

Added threaded tests (copied from nested2) and ensured that the
tests work. Required using the cwd argument locally in popen so
that different threads do not use different working directories.

Runs of gcov now lock the working directory as this is the directory
that contains the output files. There appears to be _no_ way to get
gcov to prefix its files or put them anywhere different.

This should work reasonably well for systems that use absolute paths
as a temporary directory is tried first. For relative but nested runs
this should be OK too. For other cases (g++/gcc always run in the
same folder) this is likely to fall down to single threaded.

@JamesReynolds JamesReynolds force-pushed the JamesReynolds:upstream branch from a4451e6 to 23e3b4a Mar 8, 2018

# This file is part of gcovr <http://gcovr.com/>.
#
# Copyright 2013-2018 the gcovr authors
# Copyright 2013 Sandia Corporation

This comment has been minimized.

@latk

latk Mar 8, 2018

Member

no code in this file was copied from other parts of this project, so Copyright 2018 the gcovr authors is sufficient

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

Done, will merge (won't rebase yet) when all are completed.

"""
Initialise with a reference to the pool object
which houses the queue
"""

This comment has been minimized.

@latk

latk Mar 8, 2018

Member

The constructor documentation is typically part of the class documentation, as the __init__ method is never called directly.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

Done, will merge (won't rebase yet) when all are completed.

def close(self):
"""
Empty the working directory
"""

This comment has been minimized.

@latk

latk Mar 8, 2018

Member

Must close() only be called after all threads have been joined? Or could it be part of the run() method? If so, we can turn this class into a single worker(q) function, to be started like Thread(target=worker, args=(self.q,)).

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

Done, will merge (won't rebase yet) when all are completed. Very good suggestion - that saves a fair few lines of code and makes much more sense.

Wait until all work is complete
"""
for w in self.workers:
w.start()

This comment has been minimized.

@latk

latk Mar 8, 2018

Member

Currently, the workers must start after all work items have been enqueued, as the workers exit when the queue is empty. Does this have any particular advantages or disadvantages?

The alternative pattern is to let the workers exit when they dequeue a sentinel value, e.g. None. The threads would then be started as soon as constructed, and the wait()method would for _ in self.workers: self.q.put(None).

This comment has been minimized.

@mayeut

mayeut Mar 8, 2018

Contributor

I fully agree with @latk, It would be best to start as soon as the first item is enqueued (it would be even better if get_datafiles returned a generator rather than a list, I have not checked if it's possible though).
It would also help get rid of self.q.get(False, 5) which seems dangerous (Imagine for one reason or another, e.g. the python process is denied a cpu slot for 5 seconds, what would happen).

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

Done, will merge (won't rebase yet) when all are completed. It makes the code neater to use a sentinel and it does mean that the threads start processing as soon as possible. Thinking about it both time.sleep and the timeout here are code smells I should've jumped on!

self.q = Queue()
if number == 0:
from multiprocessing import cpu_count
number = cpu_count()

This comment has been minimized.

@latk

latk Mar 8, 2018

Member

There is no test case covering the number == 0 branch, see the coverage report. Arguably, this is not necessary if we stop special-casing zero (but still make sure at least one thread exists: assert number >= 1), and instead set const=cpu_count() in the argument parsing code.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

Done, will merge (won't rebase yet) when all are completed. Again, much neater.

else:
process_datafile(file_, covdata, options)
pool.add(process_datafile, file_, covdata, options)

This comment has been minimized.

@latk

latk Mar 8, 2018

Member

The covdata dict will be updated from multiple threads, but is not synchronized. Should this access be guarded through a mutex section? Or should the workers write update commands to an output queue that is then executed on the main thread?

There may also be concurrent accesses to one covdata entry if multiple threads provide coverage for the same file, in particular for header files that are included in multiple parts of the project.

This comment has been minimized.

@mayeut

mayeut Mar 8, 2018

Contributor

Good remark, access to covdata shall be synchronized. One other possibility not mentioned by @latk is to have one covdata dict per thread and do a threaded merge at the end (nb thread/2 at each merge step).

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

I've created a LockedDictionary object that updates with a lock. As this is only called in one place after the recent refactor to gcov.py this seemed to be the easiest option.

However... keeping a context per thread and merging is a nice pattern and avoids further locks. It also may cure help the problem in the next comment...

This comment has been minimized.

@mayeut

mayeut Mar 9, 2018

Contributor

Just forget my comment regarding threaded merge. This won't do any good running only python code (without I/O). I should have known better as I always try to release the GIL in long running tasks in C extensions to allow multithreading... Python only runs python code in one thread at a time.

This comment has been minimized.

@latk

latk Mar 9, 2018

Member

@mayeut Just to clarify, you're saying “multi-level merging is pointless in Python because merging can't happen in parallel”, but not “collecting separately and merging at the end is unnecessary”, right? So the current design (with a per-thread covdata dictionary) is correct as per your experience?

This comment has been minimized.

@mayeut

mayeut Mar 9, 2018

Contributor

Right.

This comment has been minimized.

@mayeut

mayeut Mar 10, 2018

Contributor

@latk, I really need to think "python". Thinking again about your question. I did not answer correctly. So, the only thing that can benefit from threading are I/O here (subprocess being part of that). Updating the coverage dictionary is pure python with no I/O, 2 threads can't really do that in parallel (compute time wise) so, we're probably better off protecting access to the dictionary rather than merging at the end since there can't really be contention here. Merging can be easier (thread safety) but not faster.

if chdir != tempdir:
shutil.copyfile(os.path.join(chdir, fname), active_files[-1])
else:
active_files.append(os.path.join(chdir, fname))
return active_files, all_files

This comment has been minimized.

@latk

latk Mar 8, 2018

Member

active_files is the list of files that should be processed, and all_files the list of files that should be kept or deleted, depending on the --keep flag. Here, all_files only receive the fname without the chdir prefix. I think that is a bug unless chdir == ".".

Additionally, the tempdir files are not part of the all_files and will not be cleaned up until the worker exists. And we are copying and not moving the files to the tempdir even when they are guaranteed to be removed later. This isn't incorrect, but it feels inelegant. I don't know what the correct solution would be. Maybe a third return values for files that should be always deleted after processing?

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

I've fixed the all_files bug.

I think the move vs copy etc... is a bit more of a thorny issue. Windows is throwing file-in-use errors when either moving or deleting. I think the best option would be to copy (as its clear what is happening and avoids a process lock if there is one), keep track of all the files, then to delete after all the threads are complete. I'm working on making Windows happy today and this is what I'll try first.

I've also had to add exception tracking to the threads so their exceptions get raised on the main thread.

@@ -677,7 +683,7 @@ def run_gcov_and_process_files(
return done
def select_gcov_files_from_stdout(out, gcov_filter, gcov_exclude, logger):
def select_gcov_files_from_stdout(out, gcov_filter, gcov_exclude, logger, chdir, tempdir=None):

This comment has been minimized.

@latk

latk Mar 8, 2018

Member

tempdir=None never happens, as a workdir is always set when executing through the thread. I think these arguments can be made mandatory.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

Done, will merge (won't rebase yet) when all are completed.

# On Windows the files may still be in use. This
# is unlikely, the files are small, and are in a
# temporary directory so we can skip this.
shutil.rmtree(self.workdir, ignore_errors=True)

This comment has been minimized.

@mayeut

mayeut Mar 8, 2018

Contributor

Why would the files still be in use on Windows ? This doesn't seem right. Any pointers ?

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

Indeed it does not seem right - as these are just parsed by python and not touched by gcov. I think this may be able to go once I've cured the (potential) issue with deletion of the *.gcov files. I've set up a local Windows test machine now so I should get this done today.

James Reynolds
Add requested changes from code review
1. Cleanup copyright
2. Remove documentation from __init__
3. Inline the thread class into a thread function
4. Change empty queue to sentinel object
5. Remove special zero in Workers by using cpu_count in options
6. Change to per-thread covdata and set of files to erase
7. Cleanup default parameters where not required
@JamesReynolds

This comment has been minimized.

JamesReynolds commented Mar 9, 2018

@latk @mayeut I've submitted another revision (not rebased though so you can see the change) which I think addresses all the issues. I went with a sentinel object and with a per-thread context that I merge in main. It solves the deadlock and crashes on Windows and I think gives marginally better performance on Sparc too.

@latk

This is really great work, thank you so much for working through all these details!

Unfortunately, multithreaded code is like a hydra: solve one race condition and another pops up. Below, I have a few questions/observations about exception propagation.

except: # noqa: E722
import sys
exceptions.append(sys.exc_info())
break

This comment has been minimized.

@latk

latk Mar 9, 2018

Member

I'd prefer to have at least one unit test that exercises this code path. Basically, start a thread pool, add a function that throws, wait, then verify that the stderr contains a traceback and that the exception is rethrown. The test_pathological_codeline() in test_gcov_parser.py contains a similar example.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

Agreed. Job for Monday!

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 12, 2018

I've added a test, its a bit involved as I had to add some events to ensure that I could deterministically check that the queue was drained properly.

except: # noqa: E722
import sys
exceptions.append(sys.exc_info())
break

This comment has been minimized.

@latk

latk Mar 9, 2018

Member

What happens if two threads have exceptions at the same time? The exceptions is an unsynchronized list. Should it be a queue?

Maybe it is sufficient to store the exception in the thread object itself, and checking this field after joining. That would mean we reintroduce a thread subclass, though.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

I think list appends are thread-safe (http://effbot.org/zone/thread-synchronization.htm) but I'm having trouble finding a completely official line on that. If that is the case then I think two threads would always produce two, distinct, exceptions. docs.python does specifically say that a deque does do this atomically though, so I could switch to that?

This comment has been minimized.

@latk

latk Mar 9, 2018

Member

OK, that sounds reasonable. There's no reason why list.append() would have to release the GIL, and I couldn't find anything surprising in the CPython source code. So I think we can keep using the list, iff exceptions is only consumed after all threads are joined (as is the case in the current design).

except: # noqa: E722
import sys
exceptions.append(sys.exc_info())
break

This comment has been minimized.

@latk

latk Mar 9, 2018

Member

If one thread exits due to an exception, the others will continue until all work is done (or all have died). So with each exception, processing slows down further. This could e.g. happen if some gcov files have parse errors. I'm also thinking about how this integrates with Ctrl-C/KeyboardInterrupt exceptions.

Can we somehow turn this into fail-fast behaviour? E.g. if an exception is caught, the input queue is drained?

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 9, 2018

I agreed. A job for Monday - I'll make sure that if an exception is raised the queue is drained and a sentinel pushed. I think I also need to wrap the call to this in __main__.py in a context manager that can terminate threads if an exception is thrown prior to the sentinels being pushed. It would probably be neat to make the pool itself a context.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 12, 2018

This is done in the latest commit, on an exception the queue is drained and there is code to ensure that the sentinels persist - so that if the drain occurs prior to pool.wait() being called the threads still stop. I've also wrapped the pool in a context manager so that keyboard interrupts are caught and the queue drained from main.

This comment has been minimized.

@latk

latk Mar 13, 2018

Member

Thank you for these updates. At first glance this looks quite good, but a proper review will have to wait until I'm less tired.

James Reynolds
Add exception testing and allow interrupts
1. Add a test `test_pathologic_threads` to check that a
   child thread throwing an exception will be caught in
   main and that the queue of tasks is successfully dumped
2. Ensure that exceptions thrown during pool setup and wait
   cancel the threads by draining the queue
3. Reduce the scope of the directory locking
4. Ensure that sentinels are not used up to guard against
   exceptions being thrown prior to pool.wait() being called
@latk

I've found a few points, but am not sure how important they are.

My intention would be to merge this PR soonishly even if not all of my concerns are addressed, and then see if any problems crop up. It may be possible to refactor and simplify this code further at some point in the future.

# Wait until the exception has been completed
while not pool.exceptions:
time.sleep(0)

This comment has been minimized.

@latk

latk Mar 14, 2018

Member

Is sleep(0) guaranteed to yield from this thread and let other threads run? Or might this be a while True: pass style infinite loop?

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 15, 2018

It is (https://docs.python.org/3/library/sched.html) but it was a pain to find! Waiting like this is obviously not great, but I didn't want to complicate the workers code to make the test cleaner.

This comment has been minimized.

@latk

latk Mar 15, 2018

Member

Good find! You can leave this as it is, or add a comment.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 16, 2018

Added a comment.

@@ -95,7 +113,8 @@ def add(self, work, *args, **kwargs):
Add in a method and the arguments to be used
when running it
"""
self.q.put((work, args, kwargs))
if not work or not self.exceptions:
self.q.put((work, args, kwargs))

This comment has been minimized.

@latk

latk Mar 14, 2018

Member

This methods seems to do: add a work item to the queue unless there already was an exception. Always add the end sentinels. The check is not needed for correctness, but is only used as an optimization to avoid enqueueing items that would be drained immediately. Is that correct?

It might be a lot easier to understand if the end sentinels are added through a different method, and only keep the if not exceptions: put(…) check.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 15, 2018

That is correct. I can make this change if you would like? I agree it would make the code neater to have an add_sentinel method.

This comment has been minimized.

@latk

latk Mar 15, 2018

Member

Your call. Perhaps just add a comment. Thankyou for confirming my understanding.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 16, 2018

I made an add_sentinel method - it fitted well with the refactor to ensure draining and adding work couldn't conflict.

@@ -110,10 +129,19 @@ def wait(self):
for w in self.workers:
self.add(None)
for w in self.workers:
w.join()
while w.is_alive():
w.join(timeout=0.1)

This comment has been minimized.

@latk

latk Mar 14, 2018

Member

Why is it necessary to use a timeout here?

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 15, 2018

I found the answer here (https://stackoverflow.com/questions/3788208/threading-ignores-keyboardinterrupt-exception) - it was the only way I could get a KeyboardInterrupt to get reported during a join. I don't like it myself, but I think its better than making the threads daemons.

This comment has been minimized.

@latk

latk Mar 15, 2018

Member

OK – but please add that as a comment.

I think I remember a similar problem when I was doing complicated stuff with multiprocessing, which relies on threads. At the time I didn't find any obviously correct way to handle KeyboardInterrupt, and just used Perl instead.

In our case, I don't think having a low timeout is important? This should behave exactly the same when timeout=2 (for example)?

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 16, 2018

I've added a comment and gone with timeout=1, for my sparc box that means things stop within 10 seconds (the gcov run is ~8 or 9s).

assert excinfo.value.args[0] == "Number == 0"
# Fewer than 1 job per thread is executed
assert mutable[0] < threads

This comment has been minimized.

@latk

latk Mar 14, 2018

Member

I think it would be possible that exactly one job per thread is dequeued. Should the check be changed to “At most 1 job per thread”: assert mutable[0] <= threads?

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 15, 2018

That is true - but one of those jobs is to throw an exception, so it doesn't increment the counter. ...which is not even slightly explained by my comment! Again, I could change the comment prior to the merge to say "increment" instead of "job"?

if number == 0:
raise Exception("Number == 0")
exc_raised.wait()
total[0] += 1

This comment has been minimized.

@latk

latk Mar 14, 2018

Member

Increments are not threadsafe. In this case it's not strictly important as we are only asserting an upper bound on this count. However, it might be better to append an item to this array here, and use the length of the array later.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 15, 2018

That is true - but it could mean that we miss failed tests if something goes wrong in the future?

This comment has been minimized.

@latk

latk Mar 15, 2018

Member

I have a preference for correct code because it prevents “WTF!!?!”s in the future. In this case, either a fix or a comment would be fine.

sentinel_count = 0
while True:
try:
work, args, kwargs = queue.get(False)

This comment has been minimized.

@latk

latk Mar 14, 2018

Member

Is it correct to use nonblocking operations here? I think that would make it possible that items are added after this loop completes and before sentinels are enqueued.

Instead, it might be better to block until we get a sentinel? Or could that lead to deadlocks, which are worse than a few extra items? Or should we enqueue a sentinel before we start draining it. Or should we introduce a condition variable that controls whether the queue is open? That condvar would then be set as soon as an exception is received.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 15, 2018

I'm not sure about this myself. I was aiming for simplicity with a possibility of some extra work in the case of error. Perhaps abstracting the error mechanism into the Workers class so that the worker asks the Workers class to register an exception. At that point the Workers class could put locks around adding work to the queue and raising errors + draining the queue.

I think if we don't allow an addition at the same time as raising an exception - and we use a separate mutex to the queue - we shouldn't get deadlock.

This comment has been minimized.

@latk

latk Mar 15, 2018

Member

I think maybe the if not exceptions test in add() actually serves as a kind of mutex and prevents enqueueing of extra items, if the exceptions is appended before draining.

In any case – you are the multithreading expert here. If you think you have a better solution it might be worth a shot.

At this point, it may be best to extract all queue operations (add, sentinel, dequeue, drain) into a separate class so that it's easier to see what is going on. But that can also be done outside of this PR.

This comment has been minimized.

@JamesReynolds

JamesReynolds Mar 16, 2018

I've moved things into the pool and supplied a lock. I agree it makes things easier to read to have the methods split up and it means that the mutex is explicit and checked properly.

@JamesReynolds

This comment has been minimized.

JamesReynolds commented Mar 15, 2018

I'm happy to do one more round of changes - but as this code is only for the error case, it may be best to refactor later. Your choice!

@latk

This comment has been minimized.

Member

latk commented Mar 15, 2018

Thank you @JamesReynolds! I'm very grateful for you doing all this work and I don't want to exhaust your motivation with unnecessary nitpicking. If you're happy to go for one more round with the above comments that would be great.

James Reynolds
Clean-up multi-threading error handling and tests
1. Comments to explain yielding and interrupting threads
2. Changed the threading test to use a list to avoid race conditions
3. Abstracted the error mechanism into the pool itself and prevented
   adding tasks conflicting with draining the queue or adding
   the sentinels.
@JamesReynolds

This comment has been minimized.

JamesReynolds commented Mar 16, 2018

No problem! I'd rather get this right and clean even if it takes a little longer. Should we rebase this down to a single change as well?

@latk

latk approved these changes Mar 16, 2018

Thank you, this looks great. I'll try to merge during the weekend.

One topic for future investigation is that the tests now seem to take 20% longer (judging from the Travis and Appveyor reports). This is more than I would have expected. This could be an artefact of the test runner, or an actual performance regression. If it's an actual problem, this might be fixable by bundling all jobs for one directory into a single job, as that reduces waiting on locks.

Can you re-verify the performance improvements on SPARC? I'm interested in wall-clock runtime/elapsed time comparison between gcovr on master and gcovr on this PR when running in parallel mode.

@JamesReynolds

This comment has been minimized.

JamesReynolds commented Mar 16, 2018

Great! I've checked our SPARC improvements and it's still good. Perhaps the additional copy (which is not required in the single threaded case) adds additional I/O overhead in the tests?

@latk latk merged commit 288fae1 into gcovr:master Mar 18, 2018

4 checks passed

codecov/patch 96.96% of diff hit (target 87.35%)
Details
codecov/project 88.71% (+1.36%) compared to f997eda
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

latk added a commit that referenced this pull request Mar 18, 2018

Merge pull request #239 from JamesReynolds
- closes issue #3 (parallelize main loop of gcovr)
- see issue #36

This was referenced Mar 18, 2018

@latk latk removed the needs review label Mar 18, 2018

@JamesReynolds JamesReynolds deleted the JamesReynolds:upstream branch Mar 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment