[google gcc-4_7] offline profile merge tool (issue 8508048)

xur@google.com xur@google.com
Wed Apr 10 08:17:00 GMT 2013


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/



More information about the Gcc-patches mailing list