macOS: disable AppNap during sync #12783

pull krab wants to merge 1 commits into bitcoin:master from krab:macos-disable-appnap changing 6 files +116 −4
  1. krab commented at 5:40 am on March 26, 2018: contributor

    Code based on pull/5804. Tested only on macOS 10.13.3 and should support 10.9+.

    What macOS versions bitcoin core currently supports?

  2. Empact commented at 5:43 am on March 26, 2018: member
    #5804
  3. fanquake added the label MacOSX on Mar 26, 2018
  4. in src/support/macos_appnap.mm:10 in e41bb4a456 outdated 
     5+#include <Foundation/Foundation.h>
 6+
 7+class CAppNapInhibitorInt
 8+{
 9+public:
10+    id<NSObject> activityId;
    jonasschnelli commented at 7:57 am on March 26, 2018:
    Is there a difference in using NSObject *activityId?
  5. in src/support/macos_appnap.mm:22 in e41bb4a456 outdated 
    17+    (NSActivityUserInitiatedAllowingIdleSystemSleep |
18+     NSActivityLatencyCritical) &
19+    ~(NSActivitySuddenTerminationDisabled |
20+      NSActivityAutomaticTerminationDisabled);
21+
22+    if ([[NSProcessInfo processInfo] respondsToSelector:@selector(beginActivityWithOptions:reason:)])
    jonasschnelli commented at 7:57 am on March 26, 2018:
    nit: code styling, use brackets {}
  6. in src/support/macos_appnap.mm:33 in e41bb4a456 outdated 
    28+{
29+#if defined(MAC_OS_X_VERSION_MAX_ALLOWED) &&  MAC_OS_X_VERSION_MAX_ALLOWED >= 1090
30+    if ([[NSProcessInfo processInfo] respondsToSelector:@selector(endActivity:)])
31+        [[NSProcessInfo processInfo] endActivity:priv->activityId];
32+#endif
33+    delete priv;
    jonasschnelli commented at 7:59 am on March 26, 2018:
    Since we are only holding a NSObject pointer, is this deallocation really required or could it even be messing with the autorelease pool?
  7. jonasschnelli commented at 7:59 am on March 26, 2018: contributor
    utACK e41bb4a4568e08fe75ab8af7de7db1a2684cd989
  8. fanquake requested review from theuni on Mar 27, 2018
  9. Sjors commented at 8:36 am on March 27, 2018: member
    Will review later. Good question regarding which macOS version to support. The release notes state 10.8+, but I wonder how true that is. Apple pushes macOS updates quite aggressively and the lack of VM’s make downgrades impractical. So unless any of the active devs here still runs an older version, I don’t think it’s realistic to support anything but the current macOS release and maybe one earlier version .
  10. jonasschnelli commented at 8:41 am on March 27, 2018: contributor
    We do currently support 10.8+ and I have VMs for all OSX versions from 10.5 upwards. I do test the RC regularly in 10.8 till 10.x VMs.
  11. Sjors commented at 9:00 am on March 27, 2018: member
    @jonasschnelli ok, that’s great to know. Any hints for setting up a 10.8 VM on an iMac?
  12. laanwj commented at 5:01 pm on March 27, 2018: member
    Concept ACK. Should appnap always be disabled, or just during initial sync? It’s something that could be done later, but worth thinking about I think.
  13. theuni commented at 10:03 pm on March 28, 2018: member

    I really really don’t like the idea of adding a new compiler requirement for bitcoind (which is why we closed #5804).

    How about using this for bitcoin-qt only ?

  14. Sjors commented at 10:44 am on March 29, 2018: member

    I think it’s reasonable* to expect bitcoind users to understand their OS power management.

  15. krab commented at 4:55 pm on March 31, 2018: contributor
    how to i squash all my commits in PR they contain merging commits from upstream/master :confused:
  16. krab force-pushed on Mar 31, 2018
  17. krab commented at 6:28 pm on March 31, 2018: contributor

    Squashed all my commits into single one.

    OS X Mountain Lion 10.8 not supported by Apple anymore. Mozilla/Google don’t support 10.8 either in their web browsers. Qt 5.7 (currently used in bitcoin-qt0.16.0) support expired in June 15, 2017. Qt 5.8 supports only 10.9+. Qt 5.9 only 10.10+. So OS X 10.8 support in bitcoin-qt pretty much pointless since it requires Qt 5.6/5.7 and very old XCode to build.

  18. laanwj commented at 1:51 pm on April 7, 2018: member

    How about using this for bitcoin-qt only ?

    Would make sense to me. Would (for me) be preferable to having objective C in the core code.

    I think it’s reasonable* to expect bitcoind users to understand their OS power management.

    Virtually no one uses bitcoind on MacOSX. For a long time, we didn’t even build it, and there was about one person that requested it :)

  19. in src/qt/macos_appnap.mm:19 in 34dfe2ce26 outdated 
    48+    NSObject* activityId;
49+};
50+
51+CAppNapInhibitor::CAppNapInhibitor(const char* strReason) : d(new Private)
52+{
53+    const NSActivityOptions activityOptions =
    theuni commented at 6:11 pm on April 9, 2018:
    Since this isn’t defined for < MAC_OS_X_VERSION_10_9, does it need a runtime check?
    krab commented at 11:34 pm on April 9, 2018:
    No idea, i can’t setup 10.8 build vm to test code properly, so pre 10.9 support removed.
  20. fanquake commented at 1:16 am on April 12, 2018: member

    Started doing some testing. Using master (979f59850c72624137d25f80be4188c3ba6b5fa0) I can make bitcoin-qt and Bitcoin Core (app bundle) enter app nap quite quickly just by starting and then minimising the app window. You can check App Nap status using Activity Monitor -> Energy.

    master - bitcoin-qt - nap master - bitcoin core - nap

    Using https://github.com/bitcoin/bitcoin/pull/12783/commits/c975861a7cf407cfd6910da691a38c4fe6491530, neither process seems to go into App Nap:

    pr - bitcoin-qt - no nap pr - bitcoin core - no nap

  21. theuni commented at 6:48 pm on April 12, 2018: member
    Concept ACK. What’s the point of MacOSIdleInhibitor though? In #5804, CIdleInhibitor was introduced as a generic object that could used to be hold any/all platform-specific inhibitors. Here, MacOSIdleInhibitor seems to just be an extra level of indirection.
  22. krab commented at 3:11 am on April 13, 2018: contributor
    Well windows and linux can’t lower cpu performance per application, on windows user can only change system-wide power plans. This thing pretty much only macos specific, MacOSIdleInhibitor change reverted anyways.
  23. krab force-pushed on Apr 14, 2018
  24. in src/qt/bitcoin.cpp:268 in e953dbfe5f outdated 
    262@@ -259,6 +263,21 @@ public Q_SLOTS:
263     void startThread();
264 };
265 
266+#if defined(Q_OS_MACOS)
267+// A RAII wrapper around calls to forbid and re-allow entering into
268+// low-priority states, specifically OSX's app-nap. For the scope of
    fanquake commented at 7:45 am on April 19, 2018:
    nit: macOS instead of OSX
  25. in src/qt/macos_appnap.mm:5 in e953dbfe5f outdated 
    0@@ -0,0 +1,44 @@
1+#include "macos_appnap.h"
    fanquake commented at 7:47 am on April 19, 2018:

    This is missing a copyright header

    0// Copyright (c) 2018 The Bitcoin Core developers
1// Distributed under the MIT software license, see the accompanying
2// file COPYING or http://www.opensource.org/licenses/mit-license.php.
  26. in src/qt/macos_appnap.h:2 in e953dbfe5f outdated 
    0@@ -0,0 +1,19 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2018 The Bitcoin Core developers
    fanquake commented at 7:48 am on April 19, 2018:

    You can just use:

    0// Copyright (c) 2018 The Bitcoin Core developers
1// Distributed under the MIT software license, see the accompanying
2// file COPYING or http://www.opensource.org/licenses/mit-license.php.

    as the copyright header here

  27. theuni commented at 3:24 pm on April 19, 2018: member
    CIdleInhibitor is still there, maybe an issue with the rebase? BitcoinCore::BitcoinCore can just create a CAppNapInhibitor.
  28. fanquake commented at 1:53 pm on May 29, 2018: member
    @krab could you now squash your commits.
  29. krab closed this on May 29, 2018
  30. krab force-pushed on May 29, 2018
  31. krab reopened this on May 29, 2018
  32. luke-jr commented at 5:11 pm on June 12, 2018: member
    Why is LSAppNapIsDisabled insufficient?
  33. theuni commented at 8:54 pm on June 12, 2018: member

    @luke-jr It’s no longer referenced in their docs, and I believe it doesn’t work at all in recent macOS versions.

    utACK, though I can’t vouch for the correctness of the ObjC inhibitor itself.

  34. fanquake commented at 4:17 pm on June 13, 2018: member

    Retested 192b9e7 on macOS 10.13.5. Cannot make Bitcoin Core enter app nap: 12783

    Master (f532d52) still enters app nap fairly quickly: master

  35. laanwj commented at 3:37 pm on July 5, 2018: member
    @krab can you take a look if you can reproduce ^^, if this works it’s ready for merge, if it doesn’t then let’s try to fix this.
  36. Sjors commented at 7:03 pm on July 5, 2018: member

    For a future PR it would be nice to disable app nap only during sync, as @laanwj points out.

    Currently the choice is between having initial sync grind (master) to a halt or wasting (a little) battery life for people who leave QT on for a long time (this PR). I think this PR is the better alternative, since it’s more realistic to exit the application to save energy than it is to keep it in the foreground to keep sync going.

    Tested: master goes into app nap after a few minutes (on battery), this branch doesn’t:

    The code looks pretty complicated, which generally suggests that Apple doesn’t like this. Do we have some confidence that we’re not using API’s that can get deprecated on short notice?

  37. krab commented at 12:40 pm on July 6, 2018: contributor

    If they changed api then code won’t even start if ([processInfo respondsToSelector:@selector(beginActivityWithOptions:reason:)]) { https://i.imgur.com/g3uKu0g.png

    it would be nice to disable app nap only during sync

    Agree, but i can’t find any signal in src/qt about “sync/initial sync” state changed. Only: BlockTipChanged and NotifyNetworkActiveChanged doesn’t seem right.

  38. in share/qt/Info.plist.in:105 in 192b9e720f outdated 
     96@@ -97,9 +97,6 @@
 97   <key>NSHighResolutionCapable</key>
 98     <string>True</string>
 99 
100-  <key>LSAppNapIsDisabled</key>
101-    <string>True</string>
102-
    luke-jr commented at 11:30 am on July 7, 2018:
    Even if it doesn’t work in latest versions, why not keep it for older ones?
    Sjors commented at 11:44 am on July 7, 2018:
    I’m having a hard time finding any Apple documentation about that flag. Do we know where it got deprecated? See also #13362
  39. in src/qt/macos_appnap.mm:22 in 192b9e720f outdated 
    17+CAppNapInhibitor::CAppNapInhibitor(const char* strReason) : d(new Private)
18+{
19+#if defined(MAC_OS_X_VERSION_MAX_ALLOWED) &&  MAC_OS_X_VERSION_MAX_ALLOWED >= 1090
20+    const NSActivityOptions activityOptions =
21+    (NSActivityUserInitiatedAllowingIdleSystemSleep |
22+     NSActivityLatencyCritical) &
    luke-jr commented at 11:35 am on July 7, 2018:
    Why latency critical?
  40. in src/qt/macos_appnap.mm:19 in 33a25f1e02 outdated 
    14+    NSObject* activityId;
15+};
16+
17+CAppNapInhibitor::CAppNapInhibitor(const char* strReason) : d(new Private)
18+{
19+#if defined(MAC_OS_X_VERSION_MAX_ALLOWED) &&  MAC_OS_X_VERSION_MAX_ALLOWED >= 1090
    fanquake commented at 2:58 pm on July 25, 2018:
    These if defs should no longer be required now that we only support 10.10+.
  41. krab force-pushed on Aug 22, 2018
  42. fanquake added this to the milestone 0.18.0 on Sep 3, 2018
  43. fanquake commented at 1:19 pm on September 3, 2018: member
    Added to the 0.18 milestone (not sure if we can add this to 0.17.1?). @krab has done a good job of keeping this rebased/updated. I’ve also done some rudimentary benchmarking, and have seen a ~20% detriment in sync times if Core falls into App Nap, which seems worth addressing.
  44. in src/qt/macos_appnap.h:11 in 787dcc31d0 outdated 
     6+#define BITCOIN_QT_MACOS_APPNAP_H
 7+
 8+class CAppNapInhibitor
 9+{
10+public:
11+    CAppNapInhibitor(const char* strReason);
    practicalswift commented at 7:42 am on September 6, 2018:
    Should be explicit? :-)
  45. fanquake added the label Needs gitian build on Oct 9, 2018
  46. fanquake commented at 5:39 am on October 9, 2018: member
    Retested that 663efef doesn’t enter app-nap. master (1d14174) still does after a short period of window minimisation. Also added needs gitian build label.
  47. in src/qt/macos_appnap.h:15 in 663efef3d7 outdated 
    10+public:
11+    explicit CAppNapInhibitor(const char* strReason);
12+    ~CAppNapInhibitor();
13+private:
14+    class Private;
15+    Private *d;
    promag commented at 0:43 am on October 10, 2018:
    nit Private* d; or std::unique_ptr<Private> d;
  48. in src/qt/macos_appnap.h:8 in 663efef3d7 outdated 
    0@@ -0,0 +1,18 @@
1+// Copyright (c) 2011-2018 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_QT_MACOS_APPNAP_H
6+#define BITCOIN_QT_MACOS_APPNAP_H
7+
8+class CAppNapInhibitor
    promag commented at 0:43 am on October 10, 2018:
    nit, final?
  49. DrahtBot commented at 0:45 am on October 10, 2018: member

    Gitian builds for commit 1d1417430c829a0c21bf5a2fe4a5b2f592a9423f (master):

    Gitian builds for commit 99e54e799f7187b97d6486e95b7544148c44a5d2 (master and this pull):

  50. DrahtBot removed the label Needs gitian build on Oct 10, 2018
  51. in src/qt/bitcoingui.h:151 in 663efef3d7 outdated 
    146@@ -143,6 +147,10 @@ class BitcoinGUI : public QMainWindow
147     HelpMessageDialog* helpMessageDialog = nullptr;
148     ModalOverlay* modalOverlay = nullptr;
149 
150+#ifdef Q_OS_MAC
151+    CAppNapInhibitor* appNapInhibitor = nullptr;
    promag commented at 0:45 am on October 10, 2018:
    Should be deleted? Maybe use std::unique. nit, m_app_nap_inhibitor;
    krab commented at 2:15 am on October 11, 2018:
    Delete what and why? 🤔
  52. promag commented at 0:48 am on October 10, 2018: member
    Concept ACK.
  53. krab force-pushed on Oct 11, 2018
  54. krab commented at 2:23 am on October 11, 2018: contributor
    Rebased against master branch. A bit less code in bitcoingui.cpp. macos_appnap.mm/h in pimpl style. Fixed checks for activityId. activityId = nil; required if we want reuse same pointer.
  55. DrahtBot commented at 11:07 am on October 20, 2018: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #14137 (gui: Add Windows taskbar progress by ken2812221)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  56. Sjors commented at 3:41 am on October 23, 2018: member

    tACK 2a14df1 on macOS 10.14 (in July I tested on 10.13)

    I previously wrote:

    it would be nice to disable app nap only during sync,

    This works now. With the app in the background, it enters app nap after a minute or so. If however I rewind the chain a bit e.g. with invalidateblock & reconsiderblock and wait for resync, it won’t enter app nap.

    It does enter app nap during expensive RPC operations (invalidateblock grinds to a halt), but let’s improve that some other time. Conversely, we could also make QT (or a future headless node) conserve energy better when the user is on battery.

    I also wrote:

    The code looks pretty complicated, which generally suggests that Apple doesn’t like this.

    The current solution looks much cleaner. E.g. it got rid of the undocumented plist option, and instead uses the documented NSActivityOptions. Some relevant Apple docs:

    For future reference, I suggest renaming the PR to “macOS: disable AppNap during sync”

  57. krab renamed this:
    macOS: Disable AppNap
    macOS: disable AppNap during sync
    on Oct 23, 2018
  58. fanquake commented at 7:14 am on October 31, 2018: member
    Related upstream ticket QTBUG-43861. If we’re lucky, one day Qt might expose an API for this functionality, so we don’t have to ship the Objective-C ourselves.
  59. DrahtBot added the label Needs rebase on Nov 1, 2018
  60. macOS: disable AppNap during sync 
    Signed-off-by: Alexey Ivanov <alexey.ivanes@gmail.com>
    1e0f3c4499
  61. krab force-pushed on Nov 1, 2018
  62. DrahtBot removed the label Needs rebase on Nov 1, 2018
  63. Sjors commented at 5:43 pm on November 2, 2018: member
    utACK 1e0f3c4 (rebased)
  64. laanwj merged this on Nov 10, 2018
  65. laanwj closed this on Nov 10, 2018
  66. laanwj referenced this in commit edc715240c on Nov 10, 2018
  67. laanwj commented at 9:40 am on November 10, 2018: member
    utACK 1e0f3c44992fb82e6bf36c2ef9277b0759c17c4c
  68. UdjinM6 referenced this in commit 779d575eaa on Jul 10, 2019
  69. UdjinM6 referenced this in commit d57cbc615b on Jul 15, 2019
  70. codablock referenced this in commit 573a372aa4 on Aug 7, 2019
  71. codablock referenced this in commit ea8569e97b on Aug 7, 2019
  72. MIPPL referenced this in commit 8ab255f49c on Nov 20, 2019
  73. barrystyle referenced this in commit 173d39ab10 on Jan 22, 2020
  74. jonasschnelli referenced this in commit feecf490c4 on May 18, 2020
  75. jonasschnelli referenced this in commit ec2c94ffea on May 18, 2020
  76. jasonbcox referenced this in commit 6105a89bcc on Nov 9, 2020
  77. DrahtBot locked this on Sep 8, 2021


krab Empact jonasschnelli Sjors laanwj theuni fanquake luke-jr practicalswift promag DrahtBot

Labels
macOS

Milestone
0.18.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me