[MacPorts] #56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[MacPorts] #56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant

MacPorts
#56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant
----------------------+---------------------
 Reporter:  RJVB      |      Owner:  (none)
     Type:  update    |     Status:  new
 Priority:  Normal    |  Milestone:
Component:  ports     |    Version:
 Keywords:  haspatch  |       Port:  leveldb
----------------------+---------------------
 This upgrade leveldb to the current git/head version 1.20.46 which gets
 rid of the need for patching the build system because the project has
 moved to using CMake.

 Leveldb will make use of tcmalloc from the gperftools port when present,
 but I found out that this can have annoying side-effects. I've been seeing
 deadlocks for instance:

 {{{
 * thread #1: tid = 0x1105e90, 0x00007fff8f75eb16
 libsystem_kernel.dylib`syscall_thread_switch + 10, queue = 'com.apple
 .main-thread', stop reason = signal SIGSTOP
   * frame #0: 0x00007fff8f75eb16
 libsystem_kernel.dylib`syscall_thread_switch + 10
     frame #1: 0x00007fff92529df6
 libsystem_platform.dylib`_OSSpinLockLockSlow + 63
     frame #2: 0x00007fff8d86890d
 libsystem_pthread.dylib`_pthread_testcancel + 28
     frame #3: 0x00007fff99864d2e libsystem_c.dylib`nanosleep + 42
     frame #4: 0x000000010b75b7b1
 libtcmalloc.4.dylib`base::internal::SpinLockDelay(int volatile*, int, int)
 + 122
     frame #5: 0x000000010b75b6af libtcmalloc.4.dylib`SpinLock::SlowLock()
 + 41
     frame #6: 0x000000010b74fe00
 libtcmalloc.4.dylib`tcmalloc::CentralCacheLockAll() + 56
     frame #7: 0x00007fff97676cd1
 libsystem_malloc.dylib`_malloc_fork_prepare + 49
     frame #8: 0x00007fff997fa161 libsystem_c.dylib`fork + 12
     frame #9: 0x0000000106be1c63 QtCore`forkfd(flags=<unavailable>,
 ppid=0x00007fff5c5e650c) + 483 at forkfd.c:691
     frame #10: 0x0000000106bd5801
 QtCore`QProcessPrivate::startProcess(this=0x0000000129019100) + 3537 at
 qprocess_unix.cpp:471
     frame #11: 0x0000000106bd180f
 QtCore`QProcessPrivate::start(this=0x0000000129019100, mode=<unavailable>)
 + 431 at qprocess.cpp:2177
     frame #12: 0x0000000106bd14ca
 QtCore`QProcess::start(this=<unavailable>, program=<unavailable>,
 arguments=0x00000001289e5a28, mode=<unavailable>) + 154 at
 qprocess.cpp:2089
 SNIP
 }}}

 Citing a Qt dev's assessment of this
 > This is the "atfork" code, the one that causes all malloc locks to be
 dropped
 > in the child process so that malloc() will work there regardless of the
 state
 > of the parent. The fact that it's deadlocking in the code that is
 supposed to
 > remove locks so that it won't deadlock is a good irony.

 This could be a system bug triggered by the fact that linking to
 libtcmalloc.dylib replaces the system malloc routines, it could also be
 simply be the result of loading a plugin that uses leveldb and thus loads
 libtcmalloc.
 The gperftools instructions warn specifically against loading libtcmalloc
 dynamically because replacing the system malloc is safe only when done
 from the onset. It is NOT safe to manage allocated memory via both set of
 routines.

 I think thus that it would be better to disable tcmalloc support in
 leveldb by default, and provide it as a build variant.

--
Ticket URL: <https://trac.macports.org/ticket/56288>
MacPorts <https://www.macports.org/>
Ports system for macOS
Reply | Threaded
Open this post in threaded view
|

Re: [MacPorts] #56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant

MacPorts
#56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant
----------------------+----------------------
  Reporter:  RJVB     |      Owner:  (none)
      Type:  update   |     Status:  new
  Priority:  Normal   |  Milestone:
 Component:  ports    |    Version:
Resolution:           |   Keywords:  haspatch
      Port:  leveldb  |
----------------------+----------------------
Changes (by RJVB):

 * Attachment "leveldb.diff" added.


--
Ticket URL: <https://trac.macports.org/ticket/56288>
MacPorts <https://www.macports.org/>
Ports system for macOS
Reply | Threaded
Open this post in threaded view
|

Re: [MacPorts] #56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant

MacPorts
In reply to this post by MacPorts
#56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant
----------------------+----------------------
  Reporter:  RJVB     |      Owner:  (none)
      Type:  update   |     Status:  new
  Priority:  Normal   |  Milestone:
 Component:  ports    |    Version:
Resolution:           |   Keywords:  haspatch
      Port:  leveldb  |
----------------------+----------------------

Comment (by ryandesign):

 I'm planning to wait for the next stable release before updating the port.

 You said in your proposed variant description that "leveldb uses tcmalloc
 (but also all applications using leveldb)". So it sounds like this feature
 infects any other ports that might be built using leveldb, and therefore I
 will probably want to disable it unconditionally and not offer the user a
 way to opt into that infection.

--
Ticket URL: <https://trac.macports.org/ticket/56288#comment:1>
MacPorts <https://www.macports.org/>
Ports system for macOS
Reply | Threaded
Open this post in threaded view
|

Re: [MacPorts] #56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant

MacPorts
In reply to this post by MacPorts
#56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant
----------------------+----------------------
  Reporter:  RJVB     |      Owner:  (none)
      Type:  update   |     Status:  new
  Priority:  Normal   |  Milestone:
 Component:  ports    |    Version:
Resolution:           |   Keywords:  haspatch
      Port:  leveldb  |
----------------------+----------------------

Comment (by RJVB):

 I'd argue against that. Tcmalloc does indeed replace the system malloc but
 it's also a *lot* faster - which is why leveldb uses it in the first
 place. Currently leveldb is a dependency only of QtWebEngine and QtWebKit
 (AFAICT) and there it clearly doesn't pose a problem (supposing official
 builds of Chromium-based browsers use a standard leveldb build).

 That's why I proposed a build variant and I've even hesitated making it
 the default because I have just no way of knowing if the deadlock I was
 seeing (NOT in an existing, unmodified port) wasn't in fact triggered by
 an issue in my code. WebEngine is slow enough as it is, so it would
 certainly benefit from any optimisation it can get. Has there ever been a
 report of problems caused by tcmalloc?

--
Ticket URL: <https://trac.macports.org/ticket/56288#comment:2>
MacPorts <https://www.macports.org/>
Ports system for macOS
Reply | Threaded
Open this post in threaded view
|

Re: [MacPorts] #56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant

MacPorts
In reply to this post by MacPorts
#56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant
----------------------+----------------------
  Reporter:  RJVB     |      Owner:  (none)
      Type:  update   |     Status:  new
  Priority:  Normal   |  Milestone:
 Component:  ports    |    Version:
Resolution:           |   Keywords:  haspatch
      Port:  leveldb  |
----------------------+----------------------

Comment (by ryandesign):

 I wasn't aware of the existence of tcmalloc or leveldb's use of it until
 you brought it up in this ticket.

 I just don't want a port to have a variant that causes other ports that
 use that variant to build themselves differently. That's a pain to handle
 correctly, as the x11/quartz situation in MacPorts demonstrates.

--
Ticket URL: <https://trac.macports.org/ticket/56288#comment:3>
MacPorts <https://www.macports.org/>
Ports system for macOS
Reply | Threaded
Open this post in threaded view
|

Re: [MacPorts] #56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant

MacPorts
In reply to this post by MacPorts
#56288: port:leveldb : minor upgrade with build system fixes and tcmalloc variant
----------------------+----------------------
  Reporter:  RJVB     |      Owner:  (none)
      Type:  update   |     Status:  new
  Priority:  Normal   |  Milestone:
 Component:  ports    |    Version:
Resolution:           |   Keywords:  haspatch
      Port:  leveldb  |
----------------------+----------------------

Comment (by RJVB):

 leveldb has been depending on gperftools since the beginning, so you could
 have known :)

 And the tcmalloc dependency is purely private to leveldb, there are no ABI
 differences, in fact, even the leveldb code doesn't contain anything
 that's tcmalloc-specific. My patch only disables the linking-in of
 tcmalloc; the current variant is no different than the optimisation (or
 debug!) variants other ports propose.

 You can get back the "stock" leveldb behaviour by doing the equivalent of
 `LD_PRELOAD=libtcmalloc`.

 Maybe read up a bit on the subject to make an informed decision? It'd be a
 pity to remove one of leveldb's sales arguments (its speed) just because
 it might cause side-effects. On my side I plan to sort this out further.
 Again, the issue might be caused by an issue in code I'm working on and/or
 a system bug in OS X 10.9 .

--
Ticket URL: <https://trac.macports.org/ticket/56288#comment:4>
MacPorts <https://www.macports.org/>
Ports system for macOS