Quiet the logcat spam caused by the process priority adjustment functions

RESOLVED FIXED in mozilla34

Status

()

Core
Hardware Abstraction Layer (HAL)
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gsvelto, Assigned: gsvelto)

Tracking

Trunk
mozilla34
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
SetProcessPriority(), SetThreadPriority() and friends have been spamming the logcat pretty much since their inception. They should be adjusted to use the HAL_LOG() instead of calling __android_log_print() directly.
(Assignee)

Comment 1

4 years ago
Created attachment 8463878 [details] [diff] [review]
bug-1045524-quiet-logcat.patch

This has been driving nuts when looking at logcat output pretty much since the beginning of the project. It's time we get rid of this spam :)
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8463878 - Flags: review?(dhylands)
Comment on attachment 8463878 [details] [diff] [review]
bug-1045524-quiet-logcat.patch

Review of attachment 8463878 [details] [diff] [review]:
-----------------------------------------------------------------

It feels like some of these are errors that we should probably be concerned about.

I also see that HAL_LOG logs everything as PR_DEBUG.

So it almost feels like we should introduce a HAL_ERR which at least uses PR_ERROR (I'll leave that up to you for now).

I also think that (as part of a separate bug) we should have a mechanism to redirect PR_LOG to logcat rather than requiring a file.
Attachment #8463878 - Flags: review?(dhylands) → review+
(Assignee)

Comment 3

4 years ago
Thanks for the review!

(In reply to Dave Hylands [:dhylands] from comment #2)
> It feels like some of these are errors that we should probably be concerned
> about.
> 
> I also see that HAL_LOG logs everything as PR_DEBUG.
> 
> So it almost feels like we should introduce a HAL_ERR which at least uses
> PR_ERROR (I'll leave that up to you for now).

That sounds like a good idea and doing it here is as good as in a follow up so I'll spin up another patch.

> I also think that (as part of a separate bug) we should have a mechanism to
> redirect PR_LOG to logcat rather than requiring a file.

Yes, absolutely. This would require some NSPR changes and I'm already relatively familiar with that so I should be able to do it quickly. BTW we probably have more bugs on the topic including bug 881389 where :jlebar had already done some quite remarkable WIP work to replace PR_LOG() (almost) entirely.
(Assignee)

Comment 4

4 years ago
Created attachment 8464740 [details] [diff] [review]
[PATCH v2] Make the priority adjustment functions use HAL_LOG() instead of spamming the logcat directly

Here's the updated patches, I've put the HAL_ERR() calls only where we're asserting or where we hit an obvious error that should always be reported. I've left all the PID/priority handling with HAL_LOG() because that's prone to fail due to race conditions and we know those are harmless.
Attachment #8463878 - Attachment is obsolete: true
Attachment #8464740 - Flags: review?(dhylands)
(Assignee)

Comment 5

4 years ago
I've done some investigation on redirecting PR_LOG()'s output to the logcat and as it turns out it's already supported. Support was introduced in bug 578496. I'll play with it a bit to see how to use it; if I manage to get it working I'll add the relevant information to MDN because this can be quite useful when debugging issues and a lot more practical than having to rely on a file stored on the device.
Comment on attachment 8464740 [details] [diff] [review]
[PATCH v2] Make the priority adjustment functions use HAL_LOG() instead of spamming the logcat directly

Review of attachment 8464740 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: hal/gonk/GonkHal.cpp
@@ +1395,5 @@
>  {
>    errno = 0;
>    int origProcPriority = getpriority(PRIO_PROCESS, aPid);
>    if (errno) {
> +    HAL_LOG(("Unable to get nice for pid=%d; error %d.  SetNiceForPid bailing.",

So I didn't notice this before. What's with the double parens?

I would have thought that doing something like:

#define HAL_LOG(msg, ...) PR_LOG(mozilla::hal::GetHalLog(), PR_LOG_DEBUG, (__VA_ARGS__))

would be cleaner? Or maybe this is because not all of the compilers support this?
Attachment #8464740 - Flags: review?(dhylands) → review+
(Assignee)

Comment 7

4 years ago
Ready to land, the try tun is here https://tbpl.mozilla.org/?tree=Try&rev=f68a8818b50d

(In reply to Dave Hylands [:dhylands] from comment #6)
> So I didn't notice this before. What's with the double parens?
> 
> I would have thought that doing something like:
> 
> #define HAL_LOG(msg, ...) PR_LOG(mozilla::hal::GetHalLog(), PR_LOG_DEBUG,
> (__VA_ARGS__))
> 
> would be cleaner? Or maybe this is because not all of the compilers support
> this?

I've traced back this code all the way to bug 679966 and there's no mention on why it was done this way. Support for __VA_ARGS__ in C is pretty old but since it was introduced in C++ officially only in the C++11 revision it might be possible that some compilers then would refuse it when in C++ mode in spite of already supporting it for C. I'll file a follow up to verify if we can clean this up and test it across our different builds.
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Blocks: 1047277
https://hg.mozilla.org/mozilla-central/rev/6ff5cb4b5435
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.