This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [google gcc-4_7] offline profile merge tool (issue 8508048)
- From: xur at google dot com
- To: davidxl at google dot com, tejohnson at google dot com, martint at google dot com, simonb at google dot com
- Cc: gcc-patches at gcc dot gnu dot org, reply at codereview-hr dot appspotmail dot com
- Date: Tue, 09 Apr 2013 21:38:35 +0000
- Subject: Re: [google gcc-4_7] offline profile merge tool (issue 8508048)
- Reply-to: xur at google dot com, davidxl at google dot com, tejohnson at google dot com, martint at google dot com, simonb at google dot com, gcc-patches at gcc dot gnu dot org, reply at codereview-hr dot appspotmail dot com
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py
File contrib/profile_merge.py (right):
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode53
contrib/profile_merge.py:53: data_file = open(path, 'rb')
On 2013/04/09 17:32:11, martint wrote:
How about simpler version:
with open(file, 'rb') as f:
data = f.read()
Done.
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode59
contrib/profile_merge.py:59: def MergeCounters(objs, index,
multipliers):
changed the name to ReturnMergedCounters
On 2013/04/09 17:32:11, martint wrote:
Perhaps use function name that shows that it returns the value.
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode78
contrib/profile_merge.py:78: length: Length of the data on the disk
All the bullets sub-to Attributes are not capital.
On 2013/04/09 17:32:11, martint wrote:
Minor style. Make sure format is coherent (ends with . or not, starts
with
capital letter or not).
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode119
contrib/profile_merge.py:119: self.ident = 0
Probably not. 'reader' here is ProfileDataFile. We should have a valid
gcda/gcno file for this program. But it does not hurt to have a empty
reader. I'll leave as it's.
On 2013/04/09 17:32:11, martint wrote:
It is ever useful to have a function object with no reader?
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode138
contrib/profile_merge.py:138: return self.ArcCounters().counters[0]
that will not happen. all functions have at least one counter.
On 2013/04/09 17:32:11, martint wrote:
This will throw an exception if no counters exists. Is that expected?
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode209
contrib/profile_merge.py:209: class Lines(DataObject):
It's for reading in gcno file. We are not using this functionality for
merge.
On 2013/04/09 17:32:11, martint wrote:
Where is this used?
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1153
contrib/profile_merge.py:1153: os.chdir(direc)
On 2013/04/09 17:32:11, martint wrote:
How about using absolute paths to avoid the chdir logic?
Done.
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1199
contrib/profile_merge.py:1199: outfile_path = output_path + '/' + f
On 2013/04/09 17:32:11, martint wrote:
os.path.join() instead of +, here and other places where the pattern
is used.
Done.
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1213
contrib/profile_merge.py:1213: def Merge(self, archives, multipliers):
The caller in this program always has self in archives set.
On 2013/04/09 17:32:11, martint wrote:
The code seems to handle both the case when 'self' is inside archives
and when
it is not. Does both happen when running the code?
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1233
contrib/profile_merge.py:1233: for i, a in enumerate(archives):
i is the i-th profile archive.
On 2013/04/09 17:32:11, martint wrote:
It's hard to see what i is in this case. I would prefer to have
information
about arguments in the documentation at the top of the function, but
more
descriptive variable names and comments would also help.
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1313
contrib/profile_merge.py:1313: input_profiles[0].Merge(input_profiles,
profile_multipliers)
That will use more memory. We already use too much memory.
And it only modifies in-memory object (the profile files are not
changed).
On 2013/04/09 17:32:11, martint wrote:
Did you conside creating a new object instead of modifying the
existing?
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1317
contrib/profile_merge.py:1317: input_profiles[0].Write(output_profile)
Refer to previous comment. Only in-memory objects are overwritten.
On 2013/04/09 17:32:11, martint wrote:
Warn before overwriting?
https://codereview.appspot.com/8508048/