Merge lp:~vila/bzr/more-concurrency into lp:~bzr/bzr/trunk-old

Proposed by Vincent Ladeuil on 2009-06-04
Status: Merged
Merged at revision: not available
Proposed branch: lp:~vila/bzr/more-concurrency
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 173 lines (has conflicts)
Text conflict in NEWS
To merge this branch: bzr merge lp:~vila/bzr/more-concurrency
Reviewer Review Type Date Requested Status
John A Meinel 2009-06-04 Approve on 2009-06-04
Review via email: mp+7061@code.launchpad.net
To post a comment you must log in.
Vincent Ladeuil (vila) wrote :

Some more platforms support, a slight refactor and one smoke test.

This is built on top of John Zakmeister's previous patch.

Almost trivial, but I'd like some feedback from people running either windows or Solaris with multiple CPUs (I don't).

John A Meinel (jameinel) wrote :

Given that this isn't performance critical code, would it be more reasonable to just have a fall-through (which is what we used to have) rather than forcibly require the platform to match?

I'm just thinking that some platform strings may not be recognized. This, of course, assumes that if any attempt *works* it gives the right answer.

Also, as for NUMBER_OF_CORES, on my core2 duo I see NUMBER_OF_PROCESSORS = 2, even though it is a single physical processor w/ 2 cores. So I'm not sure that is true.

Anyway, you don't have to change anything to land it, I'm just not sure of the specific benefit of having sys.platform checks.

review: Approve
Vincent Ladeuil (vila) wrote :

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> Review: Approve

    jam> Given that this isn't performance critical code, would
    jam> it be more reasonable to just have a fall-through (which
    jam> is what we used to have) rather than forcibly require
    jam> the platform to match?

There is a fall-through if platform doesn't match.

Regarding performance, two things came to mind:

- use a dict returning the right function to call, but I thought
  it was overkill,

- caching the value, since there is no way it can change between
  several incantations (well, I think it may change on some
  high-end Solaris servers... but even there), I can tweak that
  before merging.

    jam> I'm just thinking that some platform strings may not be
    jam> recognized. This, of course, assumes that if any attempt
    jam> *works* it gives the right answer.

The whole approach (as said in the doc string) is to default to 1
if anything goes wrong.

    jam> Also, as for NUMBER_OF_CORES, on my core2 duo I see
    jam> NUMBER_OF_PROCESSORS = 2, even though it is a single
    jam> physical processor w/ 2 cores. So I'm not sure that is
    jam> true.

Ok, I'll delete the comment before merging then.

    jam> Anyway, you don't have to change anything to land it,
    jam> I'm just not sure of the specific benefit of having
    jam> sys.platform checks.

Oooh, that ! Well, I'm pretty sure some functions can be shared
between different OSes (BSD variants with sysctl comes to mind),
I thought it was just cleaner that way, cascading ifs are not
really the most optimized way :-)

I also wanted to have a test for each platform but... well, there
was little to test without duplicating each function :-/

       Vincent

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-06-05 04:50:30 +0000
3+++ NEWS 2009-06-05 08:35:26 +0000
4@@ -20,6 +20,7 @@
5 New Features
6 ************
7
8+<<<<<<< TREE
9 * ``--development7-rich-root`` is a new dev format, similar to ``--dev6``
10 but using a Revision serializer using bencode rather than XML.
11 (Jelmer Vernooij, John Arbash Meinel)
12@@ -28,6 +29,11 @@
13 configured from address. (Barry Warsaw)
14
15
16+=======
17+* The number of cores is also detected on Solaris and win32. (Vincent Ladeuil)
18+
19+
20+>>>>>>> MERGE-SOURCE
21 Improvements
22 ************
23
24
25=== modified file 'bzrlib/osutils.py'
26--- bzrlib/osutils.py 2009-05-23 04:55:52 +0000
27+++ bzrlib/osutils.py 2009-06-05 08:35:27 +0000
28@@ -38,6 +38,7 @@
29 from shutil import (
30 rmtree,
31 )
32+import subprocess
33 import tempfile
34 from tempfile import (
35 mkdtemp,
36@@ -1826,3 +1827,47 @@
37 finally:
38 termios.tcsetattr(fd, termios.TCSADRAIN, settings)
39 return ch
40+
41+
42+if sys.platform == 'linux2':
43+ def _local_concurrency():
44+ concurrency = None
45+ prefix = 'processor'
46+ for line in file('/proc/cpuinfo', 'rb'):
47+ if line.startswith(prefix):
48+ concurrency = int(line[line.find(':')+1:]) + 1
49+ return concurrency
50+elif sys.platform == 'darwin':
51+ def _local_concurrency():
52+ return subprocess.Popen(['sysctl', '-n', 'hw.availcpu'],
53+ stdout=subprocess.PIPE).communicate()[0]
54+elif sys.platform == 'sunos5':
55+ def _local_concurrency():
56+ return subprocess.Popen(['psrinfo', '-p',],
57+ stdout=subprocess.PIPE).communicate()[0]
58+elif sys.platform == "win32":
59+ def _local_concurrency():
60+ # FIXME: If NUMBER_OF_CORES is available somewhow, it will be more
61+ # precise
62+ return os.environ.get('NUMBER_OF_PROCESSORS')
63+else:
64+ def _local_concurrency():
65+ # Who knows ?
66+ return None
67+
68+
69+def local_concurrency():
70+ """Return how many processes can be run concurrently.
71+
72+ Rely on platform specific implementations and default to 1 (one) if
73+ anything goes wrong.
74+ """
75+ try:
76+ concurrency = _local_concurrency()
77+ except (OSError, IOError):
78+ concurrency = None
79+ try:
80+ concurrency = int(concurrency)
81+ except (TypeError, ValueError):
82+ concurrency = 1
83+ return concurrency
84
85=== modified file 'bzrlib/tests/__init__.py'
86--- bzrlib/tests/__init__.py 2009-06-04 21:25:46 +0000
87+++ bzrlib/tests/__init__.py 2009-06-05 08:35:27 +0000
88@@ -2753,7 +2753,7 @@
89
90
91 def fork_decorator(suite):
92- concurrency = local_concurrency()
93+ concurrency = osutils.local_concurrency()
94 if concurrency == 1:
95 return suite
96 from testtools import ConcurrentTestSuite
97@@ -2762,7 +2762,7 @@
98
99
100 def subprocess_decorator(suite):
101- concurrency = local_concurrency()
102+ concurrency = osutils.local_concurrency()
103 if concurrency == 1:
104 return suite
105 from testtools import ConcurrentTestSuite
106@@ -2954,7 +2954,7 @@
107 :return: An iterable of TestCase-like objects which can each have
108 run(result) called on them to feed tests to result.
109 """
110- concurrency = local_concurrency()
111+ concurrency = osutils.local_concurrency()
112 result = []
113 from subunit import TestProtocolClient, ProtocolTestCase
114 class TestInOtherProcess(ProtocolTestCase):
115@@ -3003,7 +3003,7 @@
116 :return: An iterable of TestCase-like objects which can each have
117 run(result) called on them to feed tests to result.
118 """
119- concurrency = local_concurrency()
120+ concurrency = osutils.local_concurrency()
121 result = []
122 from subunit import TestProtocolClient, ProtocolTestCase
123 class TestInSubprocess(ProtocolTestCase):
124@@ -3049,34 +3049,6 @@
125 return result
126
127
128-def cpucount(content):
129- lines = content.splitlines()
130- prefix = 'processor'
131- for line in lines:
132- if line.startswith(prefix):
133- concurrency = int(line[line.find(':')+1:]) + 1
134- return concurrency
135-
136-
137-def local_concurrency():
138- try:
139- content = file('/proc/cpuinfo', 'rb').read()
140- concurrency = cpucount(content)
141- return concurrency
142- except IOError:
143- pass
144-
145- try:
146- output = Popen(['sysctl', '-n', 'hw.availcpu'],
147- stdout=PIPE).communicate()[0]
148- concurrency = int(output)
149- return concurrency
150- except (OSError, IOError):
151- concurrency = 1
152-
153- return concurrency
154-
155-
156 class BZRTransformingResult(unittest.TestResult):
157
158 def __init__(self, target):
159
160=== modified file 'bzrlib/tests/test_osutils.py'
161--- bzrlib/tests/test_osutils.py 2009-05-23 04:55:52 +0000
162+++ bzrlib/tests/test_osutils.py 2009-06-05 08:35:27 +0000
163@@ -1760,3 +1760,10 @@
164 def test_os_readlink_link_decoding(self):
165 self.assertEquals(self.target.encode(osutils._fs_enc),
166 os.readlink(self.link.encode(osutils._fs_enc)))
167+
168+
169+class TestConcurrency(tests.TestCase):
170+
171+ def test_local_concurrency(self):
172+ concurrency = osutils.local_concurrency()
173+ self.assertIsInstance(concurrency, int)