Opened 11 years ago

Closed 9 years ago

Last modified 9 years ago

#6330 closed enhancement (Won't Fix)

[RFC] New ThreadedFileWriter (lockless)

Reported by: Matthias "mortalmatt" Dahl <devel@… Owned by: danielk
Priority: minor Milestone: 0.25
Component: MythTV - Recording Version: head
Severity: medium Keywords:
Cc: Stuart Auchterlonie, beirdo Ticket locked: no

Description

The last few weeks I did a lot of profiling on MythTV to find some of its hot spots and try to optimize it for better behaviour under high cpu load. Even though the TFW did not explicitly come up, mutex operations are very expensive and used heavily within MythTV.

The following is my first attempt at doing some optimization. This new TFW (for now called ThreadedFileWriterLockless?) is a replacement for the old TFW. Those are the differences:

  • no mutexes in the normal path of execution
  • use of atomic operations (requires >= Qt 4.4.x)
  • aims to be thread-safe (in contrast to the old TFW) and reentrant
  • writes to the buffer don't interrupt the disk writer (true

concurrency)

I have tested the TFWL extensively on my machine with some test programs I wrote and naturally within MythTV. All written data was 100% correct, no corruption.

The patch also adds a few lines to the libmythtv project file which basically detects if support for atomic operations is available and compiles the lockless TFW instead.

So I'd really like to ask everyone interested in testing this and reporting back. Be warned though, there is no guarantee that this implementation won't eat your kitten. You're welcome to test it but don't use it on a production system... you've been warned.

What's not yet done:

  • the realtime setting code is there but commented for the moment until I have more time to do more benchmarks if this helps or hurts
  • the RingBuffer? class still contains a rwlock for accessing the TFWL which, after further tests and some modifications to RingBuffer?, can hopefully be removed
  • further documentation of the code
  • better synchronisation for some corner cases
  • support for writing data larger than the ring buffer size

Also since not every architecture supports atomic operations in a way useful to the TFW, a compile time check needs to be added which checks if the support is native or not and compiles the appropriate TFW version. Or if the "emulation" done by Qt is fast enough or at least as fast as the old TFW, that would be unnecessary (benchmarks and tests needed).

Since this is my first bigger patch to MythTV, I tried to get as close to the implicit code style as possible. I hope it's not too awful. I based the TFWL on the original TFW to keep the public API.

Like I said, feedback is very welcome. Thanks.

Attachments (10)

locklesstfw-v1.diff (26.6 KB) - added by Matthias "mortalmatt" Dahl 11 years ago.
ThreadedFileWriterLockless? v1
locklesstfw-v2.diff (28.3 KB) - added by Matthias "mortalmatt" Dahl <devel@…> 11 years ago.
ThreadedFileWriterLockless? v2
locklesstfw-v3.diff (28.1 KB) - added by Matthias "mortalmatt" Dahl <devel@…> 11 years ago.
locklesstfw-v4.diff (22.8 KB) - added by Matthias "mortalmatt" Dahl <devel@…> 11 years ago.
locklesstfw-v5.diff (22.2 KB) - added by Matthias "mortalmatt" Dahl <devel@…> 11 years ago.
locklesstfw-v5.1.diff (22.2 KB) - added by Matthias "mortalmatt" Dahl <devel@…> 11 years ago.
locklesstfw-v5.2.diff (22.2 KB) - added by Matthias "mortalmatt" Dahl <devel@…> 10 years ago.
locklesstfw-v5.3.diff (22.4 KB) - added by Matthias Dahl <devel@…> 10 years ago.
locklesstfw-v5.3-0.22-fixes.diff (22.3 KB) - added by Yeechang Lee <ylee@…> 10 years ago.
Matthias' latest patch, updated for 0.22-fixes (And trunk?)
locklesstfw-25555.patch (22.6 KB) - added by beirdo 9 years ago.
updated to svn 25555

Download all attachments as: .zip

Change History (35)

Changed 11 years ago by Matthias "mortalmatt" Dahl

Attachment: locklesstfw-v1.diff added

comment:1 Changed 11 years ago by Stuart Auchterlonie

Cc: Stuart Auchterlonie added
Milestone: unknown0.22

Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

Attachment: locklesstfw-v2.diff added

comment:2 Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

The new patch changes the following:

  • thread safety related fixes and enhancements
  • documentation updates

The patch still has some rough edges which I am going to address in a future updates. But I think it is in a state now where it can be safely tested (no guarantees though). It might not be the most efficient version yet but I am getting there once everything is in place.

Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

Attachment: locklesstfw-v3.diff added

comment:3 Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

Changes:

  • changed fileMinWriteSize to byte granularity to match the old tfw
  • removed ugly fileMinWriteSize hack and replaced it by something less ugly :)
  • fixed a possible performance bug
  • cleaned up includes
  • removed Qt 4.4 check as much as possible, the rest will be removed once a suitable runtime detection for the QAtomic-Operations is in place

comment:4 Changed 11 years ago by eric.bosch@…

I am testing your patches as I have been experiencing occaisional lock errors on ringbuffer switches during livetv viewing. Is there any specific issues to watch out for?

comment:5 Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

I doubt those are related to the ringbuffer itself. Have you already checked bug #6098? What problems are you exactly noticing...?

If you still want to test my patch, wait for the new version which will be much more simplified and thus faster. It's been a busy day but maybe I get it done tonight.

Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

Attachment: locklesstfw-v4.diff added

comment:6 Changed 11 years ago by anonymous

Since my initial versions of the patch were based on the false requirement that the TFW needs to be thread-safe, this new version has been hugely simplified and thread safety has been dropped.

Changes:

  • simplified
  • dropped thread-safety
  • added support for writing data larger than the buffer size
  • little optimizations

My not so representative (non-io bound) artificial benchmarks show that the lockless version is roughly between 40 and 50% faster than the tranditional TFW... YMMV.

comment:7 Changed 11 years ago by eric.bosch@…

I am running latest SVN with patch v4, and so far, it seems to be stable. Are there any particular tests you'd like seen run. So far, I have not had another issue of livetv stopping at the end of a program while I have had this patch in place.

Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

Attachment: locklesstfw-v5.diff added

comment:8 Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

Changes:

  • reduced number of branches as far as possible
  • put posix_fadvise() in own thread since it could cause undesirable latency spikes
  • changed from using buffer pointers to using buffer positions (simpler)
  • optimizations and some non-critical bug fixes

The patch should hopefully near its final state now. Since it's been asked what to look out for while testing this patch, here a few remarks on that subject:

  • overall performance/behaviour compared to the traditional TFW especially under higher system load (device overruns happen faster or don't happen at all, ...)
  • even though unlikely because I test every patch quite extensively but no one is perfect: corrupted recordings, MythTV hangs related to the TFW, ...
  • everything that can be clearily catagorized as being related to the lockless tfw

By the way, special thanks to Eric Bosch for testing this. It's really appreciated.

comment:9 Changed 11 years ago by eric.bosch@…

Not a problem. Glad I can contribute. Am running v5 patch now, so far as I can tell flawlessly. I'll let you know if I run into any issues.

Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

Attachment: locklesstfw-v5.1.diff added

comment:10 Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

Changes:

  • Updated to apply against recent trunk changes

One more note: I have been running v5 for 8 weeks now myself and haven't run into a single problem. Stable and no corruptions at all.

comment:11 Changed 11 years ago by paulh

Matthias, just a thought trunk now requires a qt version >= 4.4.0 so is there a good reason to keep the old TFW around assuming the new one is as stable as you say?

comment:12 Changed 11 years ago by Matthias "mortalmatt" Dahl <devel@…>

Paul, the old code base is better tested on a wider range of platforms. I have only been able to test this very thoroughly on a x86_64 based machine which ironically uncovered a corruption bug with the old TFW.

The lockless TFW relies on the atomic operations support of Qt which is not supported natively on all platforms, meaning that they are emulated with e.g. mutexes. Even though I doubt PA-RISC, SH or Blackfin matter for MythTV, so that shouldn't be a problem.

On PowerPC, S390, MIPS and SH-4a, the atomic operations are supported natively but are not guaranteed to be wait free. I haven't been able to test on those systems, so I can't say how much that affects performance. Based on some research I did and some talking on the Qt irc channel, that won't be a problem too performance wise but I still would like to see some profiling data (old TFW vs. lockless TFW) for those systems, if they matter for MythTV... I guess PowerPC still does?

That's all that jumps to my mind actually. Like I initially said, I trust it enough to use it myself. :-)

Changed 10 years ago by Matthias "mortalmatt" Dahl <devel@…>

Attachment: locklesstfw-v5.2.diff added

comment:13 Changed 10 years ago by Matthias "mortalmatt" Dahl <devel@…>

Changes:

  • Updated for recent trunk

Note: Since the lockless TFW has been running on my system since day one and has been rock solid w/o any corruptions and problems, is there still interest in merging this to get a wider testing area? I expect no major problems... the only thing I can think of so far is that there might be problems due to the use of fadvise() instead of fsync()/fdatasync(). On my system, it's a lot better that way.

comment:14 Changed 10 years ago by Matthias "mortalmatt" Dahl <devel@…>

By "that way" I meant "this way" like in "with fadvise()". Sorry for being unclear.

comment:15 Changed 10 years ago by Janne Grunau

Component: MythTV - GeneralMythTV - Recording
Milestone: 0.220.23
Owner: changed from Isaac Richards to Janne Grunau
Status: newaccepted

Changed 10 years ago by Matthias Dahl <devel@…>

Attachment: locklesstfw-v5.3.diff added

comment:16 Changed 10 years ago by Matthias Dahl <devel@…>

Changes:

  • Updated for recent trunk@21591

comment:17 Changed 10 years ago by Stuart Auchterlonie

Milestone: 0.230.24
Owner: changed from Janne Grunau to Stuart Auchterlonie
Status: acceptedassigned

Something like this is better put in at the start of a new release cycle not at the end. Bumping to 0.24

Changed 10 years ago by Yeechang Lee <ylee@…>

Matthias' latest patch, updated for 0.22-fixes (And trunk?)

Changed 9 years ago by beirdo

Attachment: locklesstfw-25555.patch added

updated to svn 25555

comment:18 Changed 9 years ago by beirdo

Cc: beirdo added

comment:19 Changed 9 years ago by beirdo

Milestone: 0.240.25

Let's let this wait until after the next release so it can soak for an entire development cycle.

comment:20 Changed 9 years ago by stuartm

Milestone: 0.25

Milestone 0.25 deleted

comment:21 Changed 9 years ago by Stuart Auchterlonie

Owner: changed from Stuart Auchterlonie to danielk

Daniel,

I'm giving this to you as you've been playing with the TFW and i'm sure there's some stuff we can take from this.

Stuart

comment:22 Changed 9 years ago by danielk

Mathias, this is a neat piece of code. But since this didn't show up in your profiling and there is no other evidence of lock contention causing problems I'm closing this ticket. It's just a matter of spending the time validating this when the current code appears to be working satisfactorily.

comment:23 Changed 9 years ago by danielk

Resolution: Won't Fix
Status: assignedclosed

comment:24 in reply to:  22 Changed 9 years ago by Stuart Auchterlonie

Replying to danielk:

Mathias, this is a neat piece of code. But since this didn't show up in your profiling and there is no other evidence of lock contention causing problems I'm closing this ticket. It's just a matter of spending the time validating this when the current code appears to be working satisfactorily.

Daniel,

What about the corruption that Mathias noticed and mentioned in comment 12?

Stuart

comment:25 Changed 9 years ago by devel@…

Hi Stuart/Daniel?.

Sorry for my extended absence but life tends to always hit hard when the timing couldn't be any worse, so you just have to prioritize. :-(

@Stuart: The corruption was fixed w/ the current TFW when I discovered it. I basically isolated the old TFW and the lockless one, put a testing environment around it and feeded it all kinds of test data (meaning different sizes and such), so I could be sure both worked fine. That's how the corruption turned up.

@Daniel: Don't you worry. I haven't been using the lockless TFW for ages now, honestly. It was a nice project that I believed in and it gave me the proper opportunity to learn about the inner workings of MythTV. And along the way I got to fix a corruption bug. So all in all, it was worth it. :-) And you're right, as long as the old TFW does not become a bottle neck or otherwise turns out problematic, there is no point in ripping it out and replacing it by something else which is entirely new.

Thanks for everything, Matthias

Note: See TracTickets for help on using tickets.