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

easylogging++: faster access to logging #4840

Merged
merged 2 commits into from Dec 12, 2018

Conversation

3 participants
@moneromooo-monero
Contributor

moneromooo-monero commented Nov 12, 2018

If someone has a better patch, feel free.

@@ -552,7 +552,7 @@ typedef std::ostream ostream_t;
typedef unsigned int EnumType;
typedef unsigned short VerboseLevel;
typedef unsigned long int LineNumber;
typedef std::shared_ptr<base::Storage> StoragePointer;
typedef base::Storage *StoragePointer;

This comment has been minimized.

@vtnerd

vtnerd Nov 13, 2018

Contributor

This now leaks a global base::Storage object (elStorage). This exists for the lifetime of the system anyway - intentional?

This comment has been minimized.

@moneromooo-monero
{
return getresetELPP(false);
if (!el::base::elStorage)
el::base::elStorage = new el::base::Storage(el::LogBuilderPtr(new el::base::DefaultLogBuilder()));

This comment has been minimized.

@vtnerd

vtnerd Nov 13, 2018

Contributor

The construction of this variable is no longer thread-safe. Do all the apps call a log function before starting threads? Is that an acceptable risk? Accessing the thread-safe atomic (the implicit static init bool) should be fairly quick since its read-only after construction (thus no cache destruction).

The logger has many other slow-downs worth looking at, the number of cycles here should be low compared to those other issues.

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Nov 13, 2018

Contributor

At least one log is called rom a static object ctor (crypto::null_skey -> mlock -> get_page_size), so efore starting threads.
"allowed" is still showing a bit on the profile, I suspect just the locking, but that's still acceptable now. getresetELPP was just > 2%, whch seemed outrageous.

This comment has been minimized.

@vtnerd

vtnerd Nov 15, 2018

Contributor

Relying on a static object to be constructed is problematic - the process could be linked without the crypto lib. Additionally, I hope to provide a patch that removes that log before main because it is impossible to disable logging to console (useful for the light wallet server admin process which can be used by python apps or bash scripts etc).

This comment has been minimized.

@vtnerd

vtnerd Nov 15, 2018

Contributor

BTW - this probably won't cause a problem, but I'm not sure how helpful the 2% in the profile is in this case - there are log statements everywhere so this function is going to get skewed due to all of those calls. How much on aggregate do some of these functions with log statements perform now? It should be a fairly small increase overall.

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Nov 15, 2018

Contributor

I don't understand what you're asking.

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Nov 15, 2018

Contributor

If you're asking "is it slow because it's called a lot", then yes.

moneromooo-monero added some commits Nov 11, 2018

easylogging++: faster access to logging
Turns out getting the global shared_ptr hits the profile,
and passing it around still keeps it at close to ~1% CPU,
which is too much for mostly silent logging.

Leak the object instead, which is even safer for late logging.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:fl branch from 1122170 to 721aacd Nov 27, 2018

@fluffypony

Reviewed

@fluffypony fluffypony merged commit 721aacd into monero-project:master Dec 12, 2018

8 of 10 checks passed

buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fluffypony added a commit that referenced this pull request Dec 12, 2018

Merge pull request #4840
721aacd easylogging++: faster access to logging (moneromooo-monero)
7cc27b3 Revert "easylogging++: make the logger handle early/late logging" (moneromooo-monero)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment