669367 - Set CPU affinity when there are multiple cores

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

Attachment: set-cpu-affinity

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110704 Firefox/7.0a1
Build ID: 20110704030740

Steps to reproduce:

When we have multiple cores, we ought to set the CPU affinity of our processes so that the content process doesn't interrupt the chrome process.

The attached patch does as much and works on Linux and Android.

Comment 1

[Chris Lord [:cwiiis]]

Comment on attachment 543993 [details] [diff] [review]
set-cpu-affinity

The patch is missing detecting whether we have the sched_setaffinity function in configure.in (though it's not actually necessary, it would be nice) and has some debugging still in.

Submitting now to see if it's the right way to go about it and if it's worth doing.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

>When we have multiple cores, we ought to set the CPU affinity of our processes 
>so that the content process doesn't interrupt the chrome process.

I'm trying to imagine the use case where we want to do this and Linux wouldn't do it automatically. Is the Android scheduler different in this regard from the normal Linux one?

In normal circumstances, when both of our threads are eating CPU, I'd expect them to get scheduled to different CPUs.

If Android doesn't do this, for example (I have no idea if this would be the case) because it prefers to only load 1 CPU for power purposes, we can indeed get better responsiveness by doing this.

However, if we control this manually, it's possible for us to go wrong here because we don't have the complete picture the scheduler has.

Imagine the following case: we're on 2 core ARM Android device. We pin our GUI to CPU 1. Our GUI uses a tiny bit of CPU constantly. The content process is idle. The scheduler now would want to move both threads to CPU 0, because they're barely using CPU, and disable CPU 1. Oops, can't do that as we are pinned there. Fennec ends up eating the battery twice as fast.

I'd normally use sched_setaffinity in cases where I know something about the CPUs that the OS doesn't know. (i.e. Hyperthreading but not hyperthreading-aware scheduler, peculiar cache organization/grouping between CPUs).

So basically, I'd be wary of doing this unless we actually observe that Android gets it wrong. Get a dual-core device, let the renderer work, work the chrome, and check if CPU usage goes >50%. Or alternately, try the patch and see if responsiveness goes up (tricky to measure objectively).

Comment 3

[Chris Lord [:cwiiis]]

I guess the pathological case is that our chrome process is using, let's say, 50% of CPU1 and when it needs to update, our content process uses > 50% - Given that our content process isn't running the whole time, I wouldn't have thought that the second core would be fired up straight away and by that time, we've already taken the speed hit in the chrome process and our interactive performance has deteriorated.

Whether that's happening or not (or even can happen), I've not tested, but I wrote the patch to see if it made a difference to the hitches I see when updating the cache viewport with software rendering. I've been told that the buffer swap is literally just a swap, no allocation or copying involved, so there ought not to be any performance hit for a swap - but there very clearly is. This probably needs further investigation, as this patch didn't really seem to help much.

In retrospect though, I agree with you, and we probably shouldn't be trying to pre-empt the scheduler, which certainly has a better knowledge of what's going on system-wide than we do.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 543993 [details] [diff] [review]
set-cpu-affinity

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

lets get data some data that this patch is required.

Comment 5

[Doug Turner (:dougt)]

hmm.  lets get some data that SHOWS that this patch is required.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Wontfix as preliminary feedback from Chris is that this doesn't help the problematic case he was observing.