Skip to content
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

macOS: disable AppNap during sync #12783

Merged
merged 1 commit into from Nov 10, 2018

Conversation

@krab
Copy link
Contributor

krab commented Mar 26, 2018

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

What macOS versions bitcoin core currently supports?

@Empact

This comment has been minimized.

Copy link
Member

Empact commented Mar 26, 2018

@fanquake fanquake added the macOS label Mar 26, 2018

src/support/macos_appnap.mm Outdated
class CAppNapInhibitorInt
{
public:
id<NSObject> activityId;

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 26, 2018

Member

Is there a difference in using NSObject *activityId?

src/support/macos_appnap.mm Outdated
~(NSActivitySuddenTerminationDisabled |
NSActivityAutomaticTerminationDisabled);

if ([[NSProcessInfo processInfo] respondsToSelector:@selector(beginActivityWithOptions:reason:)])

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 26, 2018

Member

nit: code styling, use brackets {}

src/support/macos_appnap.mm Outdated
if ([[NSProcessInfo processInfo] respondsToSelector:@selector(endActivity:)])
[[NSProcessInfo processInfo] endActivity:priv->activityId];
#endif
delete priv;

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 26, 2018

Member

Since we are only holding a NSObject pointer, is this deallocation really required or could it even be messing with the autorelease pool?

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 26, 2018

utACK e41bb4a4568e08fe75ab8af7de7db1a2684cd989

@fanquake fanquake requested a review from theuni Mar 27, 2018

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Mar 27, 2018

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 .

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 27, 2018

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.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Mar 27, 2018

@jonasschnelli ok, that's great to know. Any hints for setting up a 10.8 VM on an iMac?

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 27, 2018

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.

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Mar 28, 2018

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 ?

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Mar 29, 2018

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

@krab

This comment has been minimized.

Copy link
Contributor Author

krab commented Mar 31, 2018

how to i squash all my commits in PR they contain merging commits from upstream/master 😕

@krab krab force-pushed the krab:macos-disable-appnap branch Mar 31, 2018

@krab

This comment has been minimized.

Copy link
Contributor Author

krab commented Mar 31, 2018

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.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 7, 2018

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 :)

src/qt/macos_appnap.mm Outdated

CAppNapInhibitor::CAppNapInhibitor(const char* strReason) : d(new Private)
{
const NSActivityOptions activityOptions =

This comment has been minimized.

@theuni

theuni Apr 9, 2018

Member

Since this isn't defined for < MAC_OS_X_VERSION_10_9, does it need a runtime check?

This comment has been minimized.

@krab

krab Apr 9, 2018

Author Contributor

No idea, i can't setup 10.8 build vm to test code properly, so pre 10.9 support removed.

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Apr 12, 2018

Started doing some testing.
Using master (979f598) 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 c975861, neither process seems to go into App Nap:

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

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Apr 12, 2018

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.

@krab

This comment has been minimized.

Copy link
Contributor Author

krab commented Apr 13, 2018

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.

@krab krab force-pushed the krab:macos-disable-appnap branch Apr 14, 2018

src/qt/bitcoin.cpp Outdated
@@ -259,6 +263,21 @@ public Q_SLOTS:
void startThread();
};

#if defined(Q_OS_MACOS)
// A RAII wrapper around calls to forbid and re-allow entering into
// low-priority states, specifically OSX's app-nap. For the scope of

This comment has been minimized.

@fanquake

fanquake Apr 19, 2018

Member

nit: macOS instead of OSX

src/qt/macos_appnap.h Outdated
@@ -0,0 +1,19 @@
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2018 The Bitcoin Core developers

This comment has been minimized.

@fanquake

fanquake Apr 19, 2018

Member

You can just use:

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

as the copyright header here

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Apr 19, 2018

CIdleInhibitor is still there, maybe an issue with the rebase? BitcoinCore::BitcoinCore can just create a CAppNapInhibitor.

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented May 29, 2018

@krab could you now squash your commits.

@krab krab closed this May 29, 2018

@krab krab force-pushed the krab:macos-disable-appnap branch to e76acf3 May 29, 2018

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Sep 3, 2018

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.

Show resolved Hide resolved src/qt/macos_appnap.h Outdated
@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Oct 9, 2018

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.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 10, 2018

Gitian builds for commit 1d14174 (master):

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

@promag
Copy link
Member

promag left a comment

Concept ACK.

src/qt/macos_appnap.h Outdated
~CAppNapInhibitor();
private:
class Private;
Private *d;

This comment has been minimized.

@promag

promag Oct 10, 2018

Member

nit Private* d; or std::unique_ptr<Private> d;

src/qt/macos_appnap.h Outdated
#ifndef BITCOIN_QT_MACOS_APPNAP_H
#define BITCOIN_QT_MACOS_APPNAP_H

class CAppNapInhibitor

This comment has been minimized.

@promag

promag Oct 10, 2018

Member

nit, final?

src/qt/bitcoingui.h Outdated
@@ -143,6 +147,10 @@ class BitcoinGUI : public QMainWindow
HelpMessageDialog* helpMessageDialog = nullptr;
ModalOverlay* modalOverlay = nullptr;

#ifdef Q_OS_MAC
CAppNapInhibitor* appNapInhibitor = nullptr;

This comment has been minimized.

@promag

promag Oct 10, 2018

Member

Should be deleted? Maybe use std::unique. nit, m_app_nap_inhibitor;

This comment has been minimized.

@krab

krab Oct 11, 2018

Author Contributor

Delete what and why? 🤔

@krab krab force-pushed the krab:macos-disable-appnap branch Oct 11, 2018

@krab

This comment has been minimized.

Copy link
Contributor Author

krab commented Oct 11, 2018

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.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 20, 2018

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.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Oct 23, 2018

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"

@krab krab changed the title macOS: Disable AppNap macOS: disable AppNap during sync Oct 23, 2018

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Oct 31, 2018

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.

macOS: disable AppNap during sync
Signed-off-by: Alexey Ivanov <alexey.ivanes@gmail.com>

@krab krab force-pushed the krab:macos-disable-appnap branch to 1e0f3c4 Nov 1, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 1, 2018

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 2, 2018

utACK 1e0f3c4 (rebased)

@laanwj laanwj merged commit 1e0f3c4 into bitcoin:master Nov 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 10, 2018

Merge #12783: macOS: disable AppNap during sync
1e0f3c4 macOS: disable AppNap during sync (Alexey Ivanov)

Pull request description:

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

  What macOS versions bitcoin core currently supports?

Tree-SHA512: 85809b8d8d8a05169437b4268988da0b7372c29c6da3223ebdc106dc16dcb6d3caa5c52ace3591467005b50a63fd8b2ab1cb071cb4f450032932df25d5063315
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 10, 2018

utACK 1e0f3c4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.