Use hash map for tracked_objects::location

Use hash map for tracked_objects::location

Accessing the Location->Birth std::map takes up about 0.5% of the CPU cycles, according to recent data from ChromeOS-wide profiling. That is a LOT for Chrome, which contains a ton of stuff.

Replacing it with a hash map cuts that down by 1/3 to 1/2. See the bug for more details.

BUG=chromium:463700
Signed-off-by: Simon Que <sque@chromium.org>;

Committed: https://crrev.com/e3b16f181ab489621ca9866f472e012e33b6e9c0
Cr-Commit-Position: refs/heads/master@{#320635}

[vadimt, 2015-02-26 00:16:24]

vadimt@chromium.org changed reviewers:
+ vadimt@chromium.org

[vadimt, 2015-02-26 00:16:25]

https://codereview.chromium.org/957573002/diff/1/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/1/base/location.cc#newcode18
base/location.cc:18: uintptr_t function_name = (uintptr_t)
location->function_name();
Same thing.

https://codereview.chromium.org/957573002/diff/1/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/1/base/location.h#newcode51
base/location.h:51: (function_name_ == other.function_name_);
There is no need to compare function name. File/Line is the unique key.

[Simon Que, 2015-02-26 02:07:54]

https://codereview.chromium.org/957573002/diff/1/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/1/base/location.cc#newcode18
base/location.cc:18: uintptr_t function_name = (uintptr_t)
location->function_name();
On 2015/02/26 00:16:24, vadimt wrote:
> Same thing.

Done.

https://codereview.chromium.org/957573002/diff/1/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/1/base/location.h#newcode51
base/location.h:51: (function_name_ == other.function_name_);
On 2015/02/26 00:16:24, vadimt wrote:
> There is no need to compare function name. File/Line is the unique key.

Done.

[danduong, 2015-02-27 00:00:58]

danduong@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2015-03-03 04:48:24]

https://codereview.chromium.org/957573002/diff/20001/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/20001/base/location.cc#newcode52
base/location.cc:52: hash += reinterpret_cast<uintptr_t>(file_name_);
This is pretty surprising for a reader -- this code implicitly assumes that all
Location objects that reference a specific file will have the exact same pointer
value for the string name of that file.  I'm not sure whether that's accurate --
it may well be -- but I'd at a minimum add an explanatory comment here.

https://codereview.chromium.org/957573002/diff/20001/base/location.cc#newcode61
base/location.cc:61: hash ^= hash >> 11;
Why are you implementing your own hash function?  Does base::HashPair() not give
you what you are looking for?  I think including a compile-time assertion that
uintptr_t is at most 64-bits is reasonable, if that's what worries you.

https://codereview.chromium.org/957573002/diff/20001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/20001/base/location.h#newcode47
base/location.h:47: // Comparator for hashmap insertion.
nit: "hashmap" -> "hash map" (applies throughout)

https://codereview.chromium.org/957573002/diff/20001/base/location.h#newcode52
base/location.h:52: (file_name_ == other.file_name_);
nit: No need for parens.

https://codereview.chromium.org/957573002/diff/20001/base/location.h#newcode67
base/location.h:67: std::size_t operator()(const Location& location) const {
nit: No need to specify "std::" in front of size_t.

https://codereview.chromium.org/957573002/diff/20001/base/location.h#newcode67
base/location.h:67: std::size_t operator()(const Location& location) const {
nit: Please pick one spelling or the other for operators -- either "operator ()
(...)" or "operator==(...)".

[Ilya Sherman, 2015-03-03 04:53:20]

Also, please file a bug for this work, and expand out the CL description so that
somebody reading it a year from now can quickly understand the motivation for
this change.

[Simon Que, 2015-03-03 23:28:00]

https://codereview.chromium.org/957573002/diff/20001/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/20001/base/location.cc#newcode52
base/location.cc:52: hash += reinterpret_cast<uintptr_t>(file_name_);
On 2015/03/03 04:48:23, Ilya Sherman wrote:
> This is pretty surprising for a reader -- this code implicitly assumes that
all
> Location objects that reference a specific file will have the exact same
pointer
> value for the string name of that file.  I'm not sure whether that's accurate
--
> it may well be -- but I'd at a minimum add an explanatory comment here.

Done.

https://codereview.chromium.org/957573002/diff/20001/base/location.cc#newcode61
base/location.cc:61: hash ^= hash >> 11;
On 2015/03/03 04:48:23, Ilya Sherman wrote:
> Why are you implementing your own hash function?  Does base::HashPair() not
give
> you what you are looking for?  I think including a compile-time assertion that
> uintptr_t is at most 64-bits is reasonable, if that's what worries you.

Done.

https://codereview.chromium.org/957573002/diff/20001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/20001/base/location.h#newcode47
base/location.h:47: // Comparator for hashmap insertion.
On 2015/03/03 04:48:23, Ilya Sherman wrote:
> nit: "hashmap" -> "hash map" (applies throughout)

Done.

https://codereview.chromium.org/957573002/diff/20001/base/location.h#newcode52
base/location.h:52: (file_name_ == other.file_name_);
On 2015/03/03 04:48:23, Ilya Sherman wrote:
> nit: No need for parens.

Done.

https://codereview.chromium.org/957573002/diff/20001/base/location.h#newcode67
base/location.h:67: std::size_t operator()(const Location& location) const {
On 2015/03/03 04:48:23, Ilya Sherman wrote:
> nit: No need to specify "std::" in front of size_t.

Done.

https://codereview.chromium.org/957573002/diff/20001/base/location.h#newcode67
base/location.h:67: std::size_t operator()(const Location& location) const {
On 2015/03/03 04:48:23, Ilya Sherman wrote:
> nit: Please pick one spelling or the other for operators -- either "operator
()
> (...)" or "operator==(...)".

Done.

[Simon Que, 2015-03-03 23:32:45]

sque@chromium.org changed reviewers:
+ danakj@chromium.org

[Ilya Sherman, 2015-03-03 23:49:22]

https://codereview.chromium.org/957573002/diff/40001/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/40001/base/location.cc#newcode55
base/location.cc:55: // the definition of FROM_HERE in logging.h, and how it is
used elsewhere.
Did you mean location.h?

https://codereview.chromium.org/957573002/diff/40001/base/location.cc#newcode60
base/location.cc:60: return BASE_HASH_NAMESPACE::hash<std::pair<uintptr_t, int>
>()(pair);
nit: No need for the extra space between "> >"

https://codereview.chromium.org/957573002/diff/40001/base/location.cc#newcode60
base/location.cc:60: return BASE_HASH_NAMESPACE::hash<std::pair<uintptr_t, int>
>()(pair);
Optional: Based on looking at other uses of BASE_HASH_NAMESPACE, I'd write this
method like so:

BASE_HASH_NAMESPACE::hash<std::pair<uintptr_t, int>> h;
return h(std::make_pair(reinterpret_cast<uintptr_t>(file_name_), line_number_));

https://codereview.chromium.org/957573002/diff/40001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/40001/base/location.h#newcode39
base/location.h:39: bool operator < (const Location& other) const {
nit: Please update the spelling for this operator as well.

https://codereview.chromium.org/957573002/diff/40001/base/location.h#newcode44
base/location.h:44: return function_name_ < other.function_name_;
Vadim, should the function_name_ be removed here as well?  Are you 100% sure
that it's guaranteed to be uniquely identified by the file and line number? 
It's possible to set a custom function name via the
FROM_HERE_WITH_EXPLICIT_FUNCTION() macro.  I suppose it would be very poor style
to have multiple FROM_HERE macros on a single line of code, though.

https://codereview.chromium.org/957573002/diff/40001/base/location.h#newcode65
base/location.h:65: // Hash operator for hashmaps.
nit: "hashmap" -> "hash map"

[vadimt, 2015-03-04 02:13:40]

https://codereview.chromium.org/957573002/diff/40001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/40001/base/location.h#newcode44
base/location.h:44: return function_name_ < other.function_name_;
Yes, this would be a poor style; not worth the perf loss.

Also, the related server code ignores function names anyways and uses only
file/line.

(And if you ask why the server code ignores the function name, the answer is
that you have to include the whole compiler (actually all version-dependent
compilers) in the current code parser to calculate the function name on the
server, and be able to decypher function name hashes on the server.)

[Simon Que, 2015-03-04 02:15:37]

https://codereview.chromium.org/957573002/diff/40001/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/40001/base/location.cc#newcode55
base/location.cc:55: // the definition of FROM_HERE in logging.h, and how it is
used elsewhere.
On 2015/03/03 23:49:22, Ilya Sherman wrote:
> Did you mean location.h?

Done.

https://codereview.chromium.org/957573002/diff/40001/base/location.cc#newcode60
base/location.cc:60: return BASE_HASH_NAMESPACE::hash<std::pair<uintptr_t, int>
>()(pair);
On 2015/03/03 23:49:22, Ilya Sherman wrote:
> nit: No need for the extra space between "> >"

Done.

https://codereview.chromium.org/957573002/diff/40001/base/location.cc#newcode60
base/location.cc:60: return BASE_HASH_NAMESPACE::hash<std::pair<uintptr_t, int>
>()(pair);
On 2015/03/03 23:49:22, Ilya Sherman wrote:
> Optional: Based on looking at other uses of BASE_HASH_NAMESPACE, I'd write
this
> method like so:
> 
> BASE_HASH_NAMESPACE::hash<std::pair<uintptr_t, int>> h;
> return h(std::make_pair(reinterpret_cast<uintptr_t>(file_name_),
line_number_));

Done.

https://codereview.chromium.org/957573002/diff/40001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/40001/base/location.h#newcode39
base/location.h:39: bool operator < (const Location& other) const {
On 2015/03/03 23:49:22, Ilya Sherman wrote:
> nit: Please update the spelling for this operator as well.

Done.

https://codereview.chromium.org/957573002/diff/40001/base/location.h#newcode65
base/location.h:65: // Hash operator for hashmaps.
On 2015/03/03 23:49:22, Ilya Sherman wrote:
> nit: "hashmap" -> "hash map"

Done.

[Ilya Sherman, 2015-03-04 02:25:26]

Thanks, Simon.

https://codereview.chromium.org/957573002/diff/60001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/60001/base/location.h#newcode44
base/location.h:44: return function_name_ < other.function_name_;
IMO we should remove the function_name_ here if we don't include it below. 
Actually, is this operator still used, or can it be removed entirely?

[Simon Que, 2015-03-04 02:36:22]

https://codereview.chromium.org/957573002/diff/60001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/60001/base/location.h#newcode44
base/location.h:44: return function_name_ < other.function_name_;
On 2015/03/04 02:25:26, Ilya Sherman wrote:
> IMO we should remove the function_name_ here if we don't include it below. 
> Actually, is this operator still used, or can it be removed entirely?

Yeah, I removed it locally and was able to build successfully. So the question
is, should we keep it in case we need to use it in a std::map again?

[Ilya Sherman, 2015-03-04 04:08:24]

https://codereview.chromium.org/957573002/diff/60001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/60001/base/location.h#newcode44
base/location.h:44: return function_name_ < other.function_name_;
On 2015/03/04 02:36:22, Simon Que wrote:
> On 2015/03/04 02:25:26, Ilya Sherman wrote:
> > IMO we should remove the function_name_ here if we don't include it below. 
> > Actually, is this operator still used, or can it be removed entirely?
> 
> Yeah, I removed it locally and was able to build successfully. So the question
> is, should we keep it in case we need to use it in a std::map again?

Let's remove it.  We can always add it back if we need it later.

[Simon Que, 2015-03-04 04:59:30]

https://codereview.chromium.org/957573002/diff/60001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/60001/base/location.h#newcode44
base/location.h:44: return function_name_ < other.function_name_;
On 2015/03/04 04:08:24, Ilya Sherman wrote:
> On 2015/03/04 02:36:22, Simon Que wrote:
> > On 2015/03/04 02:25:26, Ilya Sherman wrote:
> > > IMO we should remove the function_name_ here if we don't include it below.

> > > Actually, is this operator still used, or can it be removed entirely?
> > 
> > Yeah, I removed it locally and was able to build successfully. So the
question
> > is, should we keep it in case we need to use it in a std::map again?
> 
> Let's remove it.  We can always add it back if we need it later.

Done.

[Ilya Sherman, 2015-03-04 05:04:30]

LGTM, thanks.

[danakj, 2015-03-04 16:54:41]

On 2015/03/03 04:53:20, Ilya Sherman wrote:
> Also, please file a bug for this work, and expand out the CL description so
that
> somebody reading it a year from now can quickly understand the motivation for
> this change.

This still isn't addressed, please.

[danakj, 2015-03-04 16:56:52]

Looks like this change is motivated by performance. Can you collect performance
data to demonstrate an improvement? Please include android in your numbers.

[vadimt, 2015-03-04 18:39:53]

lgtm

[Simon Que, 2015-03-04 19:08:50]

On 2015/03/04 16:54:41, danakj wrote:
> On 2015/03/03 04:53:20, Ilya Sherman wrote:
> > Also, please file a bug for this work, and expand out the CL description so
> that
> > somebody reading it a year from now can quickly understand the motivation
for
> > this change.
> 
> This still isn't addressed, please.

It has been. I've added a BUG= for this.

[danakj, 2015-03-04 19:31:06]

On Wed, Mar 4, 2015 at 11:08 AM, <sque@chromium.org> wrote:

> On 2015/03/04 16:54:41, danakj wrote:
>
>> On 2015/03/03 04:53:20, Ilya Sherman wrote:
>> > Also, please file a bug for this work, and expand out the CL
>> description so
>> that
>> > somebody reading it a year from now can quickly understand the
>> motivation
>>
> for
>
>> > this change.
>>
>
>  This still isn't addressed, please.
>>
>
> It has been. I've added a BUG= for this.
>

The CL description should include a description too, please, explaining
what this change does and why it is being done.


>
> https://codereview.chromium.org/957573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-03-04 19:35:15]

On 2015/03/04 19:31:06, danakj wrote:
> On Wed, Mar 4, 2015 at 11:08 AM, <mailto:sque@chromium.org> wrote:
> 
> > On 2015/03/04 16:54:41, danakj wrote:
> >
> >> On 2015/03/03 04:53:20, Ilya Sherman wrote:
> >> > Also, please file a bug for this work, and expand out the CL
> >> description so
> >> that
> >> > somebody reading it a year from now can quickly understand the
> >> motivation
> >>
> > for
> >
> >> > this change.
> >>
> >
> >  This still isn't addressed, please.
> >>
> >
> > It has been. I've added a BUG= for this.
> >
> 
> The CL description should include a description too, please, explaining
> what this change does and why it is being done.

Done

[danakj, 2015-03-11 17:04:06]

https://codereview.chromium.org/957573002/diff/80001/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/80001/base/location.cc#newcode57
base/location.cc:57: BASE_HASH_NAMESPACE::hash<std::pair<uintptr_t, int>>
hash_factory;
Could this be more concisely written as:

return base::HashPair(reinterpret_cast<uintptr_t>(file_name_), line_number_);

?

[Simon Que, 2015-03-11 17:34:49]

https://codereview.chromium.org/957573002/diff/80001/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/80001/base/location.cc#newcode57
base/location.cc:57: BASE_HASH_NAMESPACE::hash<std::pair<uintptr_t, int>>
hash_factory;
On 2015/03/11 17:04:06, danakj wrote:
> Could this be more concisely written as:
> 
> return base::HashPair(reinterpret_cast<uintptr_t>(file_name_), line_number_);
> 
> ?

Done.

[danakj, 2015-03-11 17:37:41]

LGTM

https://codereview.chromium.org/957573002/diff/100001/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/100001/base/location.cc#newcode57
base/location.cc:57: return
base::HashPair(reinterpret_cast<uintptr_t>(file_name_), line_number_);
Cool, you could just do this in the operator() inline in the header, I think?

[Simon Que, 2015-03-11 17:58:52]

https://codereview.chromium.org/957573002/diff/100001/base/location.cc
File base/location.cc (right):

https://codereview.chromium.org/957573002/diff/100001/base/location.cc#newcode57
base/location.cc:57: return
base::HashPair(reinterpret_cast<uintptr_t>(file_name_), line_number_);
On 2015/03/11 17:37:41, danakj wrote:
> Cool, you could just do this in the operator() inline in the header, I think?

Done.

[danakj, 2015-03-11 18:09:14]

LGTM

[Simon Que, 2015-03-11 18:26:43]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-03-11 18:26:44]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimt@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/957573002/#ps120001
(title: "Call hash pair directly from header file")

[commit-bot: I haz the power, 2015-03-11 18:27:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/957573002/120001

[commit-bot: I haz the power, 2015-03-11 18:32:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-11 18:32:21]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Simon Que, 2015-03-11 21:38:14]

The build freezes with Patch Set 8. The static_assert apparently causes ninja
build to freeze.

[Ilya Sherman, 2015-03-11 21:41:25]

On 2015/03/11 21:38:14, Simon Que wrote:
> The build freezes with Patch Set 8. The static_assert apparently causes ninja
> build to freeze.

Why are the diffs in patch set 8 needed at all?  Can't the compiler cast things
on its own?

[Simon Que, 2015-03-11 21:42:02]

On 2015/03/11 21:41:25, Ilya Sherman wrote:
> On 2015/03/11 21:38:14, Simon Que wrote:
> > The build freezes with Patch Set 8. The static_assert apparently causes
ninja
> > build to freeze.
> 
> Why are the diffs in patch set 8 needed at all?  Can't the compiler cast
things
> on its own?

See the trybot failures earlier.

[Ilya Sherman, 2015-03-11 22:32:49]

On 2015/03/11 21:42:02, Simon Que wrote:
> On 2015/03/11 21:41:25, Ilya Sherman wrote:
> > On 2015/03/11 21:38:14, Simon Que wrote:
> > > The build freezes with Patch Set 8. The static_assert apparently causes
> ninja
> > > build to freeze.
> > 
> > Why are the diffs in patch set 8 needed at all?  Can't the compiler cast
> things
> > on its own?
> 
> See the trybot failures earlier.

Fair enough.  Can you static_cast the line number to uintptr_t type?

[Simon Que, 2015-03-11 23:04:27]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-03-11 23:04:27]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
vadimt@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/957573002/#ps160001
(title: "Cast line number to uintptr_t")

[commit-bot: I haz the power, 2015-03-11 23:05:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/957573002/160001

[Simon Que, 2015-03-11 23:09:22]

On 2015/03/11 23:05:39, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/957573002/160001

Looks like it's still failing, this time on iOS.

[commit-bot: I haz the power, 2015-03-11 23:11:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-11 23:11:41]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[danakj, 2015-03-11 23:43:39]

On Wed, Mar 11, 2015 at 2:38 PM, <sque@chromium.org> wrote:

> The build freezes with Patch Set 8. The static_assert apparently causes
> ninja
> build to freeze.
>

What platform?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-03-11 23:48:31]

Chrome OS, see go/simplechrome.

On Wed, Mar 11, 2015 at 4:43 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Mar 11, 2015 at 2:38 PM, <sque@chromium.org> wrote:
>
>> The build freezes with Patch Set 8. The static_assert apparently causes
>> ninja
>> build to freeze.
>>
>
> What platform?
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-03-11 23:50:04]

On Wed, Mar 11, 2015 at 4:48 PM, Simon Que <sque@chromium.org> wrote:

> Chrome OS, see go/simplechrome <https://goto.google.com/simplechrome>.
>

It builds ok elsewhere?


>
> On Wed, Mar 11, 2015 at 4:43 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Wed, Mar 11, 2015 at 2:38 PM, <sque@chromium.org> wrote:
>>
>>> The build freezes with Patch Set 8. The static_assert apparently causes
>>> ninja
>>> build to freeze.
>>>
>>
>> What platform?
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-03-11 23:53:15]

Haven't tried, but that is one of the existing workflows for Chrome OS.

On Wed, Mar 11, 2015 at 4:49 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Mar 11, 2015 at 4:48 PM, Simon Que <sque@chromium.org> wrote:
>
>> Chrome OS, see go/simplechrome <https://goto.google.com/simplechrome>.
>>
>
> It builds ok elsewhere?
>
>
>>
>> On Wed, Mar 11, 2015 at 4:43 PM, Dana Jansens <danakj@chromium.org>
>> wrote:
>>
>>> On Wed, Mar 11, 2015 at 2:38 PM, <sque@chromium.org> wrote:
>>>
>>>> The build freezes with Patch Set 8. The static_assert apparently causes
>>>> ninja
>>>> build to freeze.
>>>>
>>>
>>> What platform?
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-03-11 23:55:16]

Maybe +thakis has some insight.

Patchset 6 can't figure out what to convert uintptr_t to on mac/ios. Patchset 7
freezes the compiler?

[danakj, 2015-03-11 23:55:44]

On 2015/03/11 23:55:16, danakj wrote:
> Maybe +thakis has some insight.
> 
> Patchset 6 can't figure out what to convert uintptr_t to on mac/ios. Patchset
7
> freezes the compiler?

Add one to each of those numbers.

[danakj, 2015-03-11 23:56:17]

danakj@chromium.org changed reviewers:
+ thakis@chromium.org

[danakj, 2015-03-11 23:56:55]

Oh, silently failing to add people.. try again..

Maybe +thakis has some insight.

Patchset 7 can't figure out what to convert uintptr_t to on mac/ios. Patchset 8
freezes the compiler?

[Ilya Sherman, 2015-03-12 00:03:32]

On 2015/03/11 23:09:22, Simon Que wrote:
> On 2015/03/11 23:05:39, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/957573002/160001
> 
> Looks like it's still failing, this time on iOS.

Wat.  I admit I'm not an expert on resolution and disambiguation rules in C++,
but I'm really surprised that the call is ambiguous when the parameters are of
the same type.  Do you happen to know how uintptr_t is defined on iOS?

[Simon Que, 2015-03-12 00:29:11]

On 2015/03/12 00:03:32, Ilya Sherman wrote:
> On 2015/03/11 23:09:22, Simon Que wrote:
> > On 2015/03/11 23:05:39, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/957573002/160001
> > 
> > Looks like it's still failing, this time on iOS.
> 
> Wat.  I admit I'm not an expert on resolution and disambiguation rules in C++,
> but I'm really surprised that the call is ambiguous when the parameters are of
> the same type.  Do you happen to know how uintptr_t is defined on iOS?

It appears to fail on both Mac and iOS, just like before.

No, I don't know.

[Simon Que, 2015-03-12 00:38:06]

What if we just forced it to be uint32 on all systems? The ptr is not going
to be all 64 bits anyway, and the upper 32 bits will likely be the same for
all Chrome pointers.

Alternatively, we can use a #ifdef for Mac and iOS to do just that, while
using disambiguation for other platforms.

On Wed, Mar 11, 2015 at 5:29 PM, <sque@chromium.org> wrote:

> On 2015/03/12 00:03:32, Ilya Sherman wrote:
>
>> On 2015/03/11 23:09:22, Simon Que wrote:
>> > On 2015/03/11 23:05:39, I haz the power (commit-bot) wrote:
>> > > CQ is trying da patch. Follow status at
>> > >  https://chromium-cq-status.appspot.com/patch-status/957573002/160001
>> >
>> > Looks like it's still failing, this time on iOS.
>>
>
>  Wat.  I admit I'm not an expert on resolution and disambiguation rules in
>> C++,
>> but I'm really surprised that the call is ambiguous when the parameters
>> are of
>> the same type.  Do you happen to know how uintptr_t is defined on iOS?
>>
>
> It appears to fail on both Mac and iOS, just like before.
>
> No, I don't know.
>
> https://codereview.chromium.org/957573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-03-12 00:39:49]

On Wed, Mar 11, 2015 at 5:37 PM, Simon Que <sque@chromium.org> wrote:

> What if we just forced it to be uint32 on all systems? The ptr is not
> going to be all 64 bits anyway, and the upper 32 bits will likely be the
> same for all Chrome pointers.
>

That sounds dangerous/can give wrong/misleading data to developers
debugging things.

Alternatively, we can use a #ifdef for Mac and iOS to do just that, while
> using disambiguation for other platforms.
>

That sounds dangerous/can give wrong/misleading data to developers
debugging things on mac.


>
> On Wed, Mar 11, 2015 at 5:29 PM, <sque@chromium.org> wrote:
>
>> On 2015/03/12 00:03:32, Ilya Sherman wrote:
>>
>>> On 2015/03/11 23:09:22, Simon Que wrote:
>>> > On 2015/03/11 23:05:39, I haz the power (commit-bot) wrote:
>>> > > CQ is trying da patch. Follow status at
>>> > >  https://chromium-cq-status.appspot.com/patch-status/
>>> 957573002/160001
>>> >
>>> > Looks like it's still failing, this time on iOS.
>>>
>>
>>  Wat.  I admit I'm not an expert on resolution and disambiguation rules
>>> in C++,
>>> but I'm really surprised that the call is ambiguous when the parameters
>>> are of
>>> the same type.  Do you happen to know how uintptr_t is defined on iOS?
>>>
>>
>> It appears to fail on both Mac and iOS, just like before.
>>
>> No, I don't know.
>>
>> https://codereview.chromium.org/957573002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-03-12 03:13:28]

My guess for what's happening in patch set 7 is that we somehow pick up
something that (incorrectly) typedefs int32_t as long instead of int (e.g.
https://code.google.com/p/chromium/codesearch#chromium/usr/include/GL/glext.h...),
then when the HashPair() functions in hash_tables.h get defined, the uint64,
int32 version becomes (unsigned long long, long), and then when you try to call
it with uintptr_t (which is uin64_t aka unsigned long long on max) and int,
that's not an exact match and you get the diag.

The fix is to not let random headers redefine int32 if that's truly the cause.
(To test this theory, I suppose static_cast<int32>(location.line_number()) might
do the trick?)

If patch set 8 hangs the compiler, please file a bug for that and cc
hans@chromium and me.

[danakj, 2015-03-12 16:52:32]

On Wed, Mar 11, 2015 at 8:13 PM, <thakis@chromium.org> wrote:

> My guess for what's happening in patch set 7 is that we somehow pick up
> something that (incorrectly) typedefs int32_t as long instead of int (e.g.
> https://code.google.com/p/chromium/codesearch#chromium/
> usr/include/GL/glext.h&l=1390),
> then when the HashPair() functions in hash_tables.h get defined, the
> uint64,
> int32 version becomes (unsigned long long, long), and then when you try to
> call
> it with uintptr_t (which is uin64_t aka unsigned long long on max) and int,
> that's not an exact match and you get the diag.
>
> The fix is to not let random headers redefine int32 if that's truly the
> cause.
> (To test this theory, I suppose static_cast<int32>(location.line_number())
> might
> do the trick?)
>

If that doesn't work, does putting it back in the .cc file happen to work?

If patch set 8 hangs the compiler, please file a bug for that and cc
> hans@chromium and me.
>
> https://codereview.chromium.org/957573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-03-12 17:50:03]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-03-12 17:50:04]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
vadimt@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/957573002/#ps180001
(title: "Explicitly cast line number to uint32_t")

[commit-bot: I haz the power, 2015-03-12 17:53:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/957573002/180001

[Nico, 2015-03-12 17:58:20]

Alright, I'll patch this in and debug a bit. Mysterious.

[commit-bot: I haz the power, 2015-03-12 18:04:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-12 18:04:21]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Nico, 2015-03-12 18:15:46]

Ok, here's what's happening:

# 30
"/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/include/sys/_types/_uintptr_t.h"
3 4
typedef unsigned long uintptr_t;


OS X (and most systems except windows) use LP64, meaning longs and pointers are
64 bit, so the typedef above is strictly speaking not wrong
(http://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models has more
information, but seems down at the moment).

The hash function on the other hand thinks that a 64-bit uint should be
`unsigned long long`, and `unsigned long` and `unsigned long long` aren't
considered equivalent even if they happen to have the same bit width. So you
have to

  static_cast<uint64_t>(reinterpret_cast<uintptr_t>(...))

and then things should work. (It's overkill on 32-bit archs, but it's likely
also not a big deal to do that everywhere.) You don't need to cast line_number
to anything after all.

[danakj, 2015-03-12 18:19:44]

Thanks for the help, Nico.

On Thu, Mar 12, 2015 at 11:15 AM, <thakis@chromium.org> wrote:

> Ok, here's what's happening:
>
> # 30
> "/Applications/Xcode.app/Contents/Developer/Platforms/
> MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/
> include/sys/_types/_uintptr_t.h"
> 3 4
> typedef unsigned long uintptr_t;
>
>
> OS X (and most systems except windows) use LP64, meaning longs and
> pointers are
> 64 bit, so the typedef above is strictly speaking not wrong
> (http://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models has more
> information, but seems down at the moment).
>
> The hash function on the other hand thinks that a 64-bit uint should be
> `unsigned long long`, and `unsigned long` and `unsigned long long` aren't
> considered equivalent even if they happen to have the same bit width. So
> you
> have to
>
>   static_cast<uint64_t>(reinterpret_cast<uintptr_t>(...))
>
> and then things should work. (It's overkill on 32-bit archs, but it's
> likely
> also not a big deal to do that everywhere.) You don't need to cast
> line_number
> to anything after all.
>
> https://codereview.chromium.org/957573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-03-12 23:45:47]

On 2015/03/12 18:15:46, Nico (traveling) wrote:
>   static_cast<uint64_t>(reinterpret_cast<uintptr_t>(...))
> 
> and then things should work. (It's overkill on 32-bit archs, but it's likely
> also not a big deal to do that everywhere.) You don't need to cast line_number
> to anything after all.

Why not just directly cast the filename pointer to a uint64_t?

[Nico, 2015-03-13 00:41:09]

that should work too

On Thu, Mar 12, 2015 at 7:45 PM, <sque@chromium.org> wrote:

> On 2015/03/12 18:15:46, Nico (traveling) wrote:
>
>>    static_cast<uint64_t>(reinterpret_cast<uintptr_t>(...))
>>
>
>  and then things should work. (It's overkill on 32-bit archs, but it's
>> likely
>> also not a big deal to do that everywhere.) You don't need to cast
>> line_number
>> to anything after all.
>>
>
> Why not just directly cast the filename pointer to a uint64_t?
>
> https://codereview.chromium.org/957573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-03-13 17:43:40]

https://codereview.chromium.org/957573002/diff/180001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/180001/base/location.h#newcode62
base/location.h:62: return
base::HashPair(reinterpret_cast<uintptr_t>(location.file_name()),
When you do said cast can you add a comment explaining why it's a int64 instead
of a uintptr_t too?

[Simon Que, 2015-03-13 21:28:47]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-03-13 21:28:48]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
vadimt@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/957573002/#ps200001
(title: "explicit cast of file ptr to uint64_t")

[commit-bot: I haz the power, 2015-03-13 21:29:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/957573002/200001

[danakj, 2015-03-13 22:05:06]

LGTM

https://codereview.chromium.org/957573002/diff/200001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/200001/base/location.h#newcode64
base/location.h:64: // pltforms. The solution is to explicitly it to a uint64_t.
a couple typos: "pltforms" "to explicitly it"

[Simon Que, 2015-03-13 23:32:25]

The CQ bit was unchecked by sque@chromium.org

[Simon Que, 2015-03-13 23:36:33]

https://codereview.chromium.org/957573002/diff/200001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/200001/base/location.h#newcode64
base/location.h:64: // pltforms. The solution is to explicitly it to a uint64_t.
On 2015/03/13 22:05:06, danakj wrote:
> a couple typos: "pltforms" "to explicitly it" 

Done.

[Simon Que, 2015-03-13 23:36:33]

https://codereview.chromium.org/957573002/diff/200001/base/location.h
File base/location.h (right):

https://codereview.chromium.org/957573002/diff/200001/base/location.h#newcode64
base/location.h:64: // pltforms. The solution is to explicitly it to a uint64_t.
On 2015/03/13 22:05:06, danakj wrote:
> a couple typos: "pltforms" "to explicitly it" 

Done.

[Simon Que, 2015-03-13 23:47:37]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-03-13 23:47:37]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
vadimt@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/957573002/#ps220001
(title: "Fixed typos in comment")

[commit-bot: I haz the power, 2015-03-13 23:48:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/957573002/220001

[commit-bot: I haz the power, 2015-03-14 02:39:15]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-03-14 02:40:06]

Patchset 12 (id:??) landed as
https://crrev.com/e3b16f181ab489621ca9866f472e012e33b6e9c0
Cr-Commit-Position: refs/heads/master@{#320635}