blink_gc_plugin: Make RecordInfo::Get{Fields,Bases} return deterministic order

blink_gc_plugin: Make RecordInfo::Get{Fields,Bases} return deterministic order

Tests are failing on the Windows bots because diagnostics get emitted in
non-deterministic order. This is a follow-up to
https://codereview.chromium.org/1645613003/ which fixed one instance, but
there were several more.

This should fix it for real by storing bases in a std::vector and using a
SourceLocation-based comparator for the fields map.

BUG=581782
R=sigbjornf@opera.com, thakis@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/8734904e17cdd88103ffce6f9da1473f48e24b45

[hans, 2016-01-29 03:00:52]

hans@chromium.org changed reviewers:
+ sigbjornf@opera.com, thakis@chromium.org

[hans, 2016-01-29 03:00:52]

Please take a look.

[Nico, 2016-01-29 03:10:08]

I'm fine with this as-is, but let's see what sof says.

https://codereview.chromium.org/1645763004/diff/1/tools/clang/blink_gc_plugin...
File tools/clang/blink_gc_plugin/RecordInfo.h (left):

https://codereview.chromium.org/1645763004/diff/1/tools/clang/blink_gc_plugin...
tools/clang/blink_gc_plugin/RecordInfo.h:72: typedef
std::map<clang::CXXRecordDecl*, BasePoint> Bases;
would giving the map a custom comparator that compares on something more
deterministic that the pointer address help as well?

[hans, 2016-01-29 04:49:14]

https://codereview.chromium.org/1645763004/diff/1/tools/clang/blink_gc_plugin...
File tools/clang/blink_gc_plugin/RecordInfo.h (left):

https://codereview.chromium.org/1645763004/diff/1/tools/clang/blink_gc_plugin...
tools/clang/blink_gc_plugin/RecordInfo.h:72: typedef
std::map<clang::CXXRecordDecl*, BasePoint> Bases;
On 2016/01/29 03:10:08, Nico wrote:
> would giving the map a custom comparator that compares on something more
> deterministic that the pointer address help as well?

For Bases, a map doesn't seem to buy as anything at all, since the callers of
GetBases() just want to iterate over all of them anyway. A vector is more
efficient for that.

For Fields, a map with a comparator that compares the Decls' SourceLocations
would work. I suppose the best solution depends on what the primary use is of
the data structure. Is it to iterate over all the fields, or to look up a
specific one? The code does both, but I don't know it well enough to say what's
more important.

Using LLVM's MapVector could also be a good alternative.

I don't care much how we solve this, as long as the iteration order is
deterministic. I suppose it's up to sof :-)

[sof, 2016-01-29 09:00:19]

https://codereview.chromium.org/1645763004/diff/1/tools/clang/blink_gc_plugin...
File tools/clang/blink_gc_plugin/RecordInfo.h (left):

https://codereview.chromium.org/1645763004/diff/1/tools/clang/blink_gc_plugin...
tools/clang/blink_gc_plugin/RecordInfo.h:72: typedef
std::map<clang::CXXRecordDecl*, BasePoint> Bases;
On 2016/01/29 04:49:14, hans wrote:
> On 2016/01/29 03:10:08, Nico wrote:
> > would giving the map a custom comparator that compares on something more
> > deterministic that the pointer address help as well?
> 
> For Bases, a map doesn't seem to buy as anything at all, since the callers of
> GetBases() just want to iterate over all of them anyway. A vector is more
> efficient for that.
> 
> For Fields, a map with a comparator that compares the Decls' SourceLocations
> would work. I suppose the best solution depends on what the primary use is of
> the data structure. Is it to iterate over all the fields, or to look up a
> specific one? The code does both, but I don't know it well enough to say
what's
> more important.
> 
> Using LLVM's MapVector could also be a good alternative.
> 
> I don't care much how we solve this, as long as the iteration order is
> deterministic. I suppose it's up to sof :-)

What you propose makes the most sense to me, use a std::vector for bases, but a
map with a Compare that orders on getLocStart(), or something. Not a very
expensive node comparison nor will the underlying tree come out the worse for
it.

[hans, 2016-01-29 17:59:47]

On 2016/01/29 09:00:19, sof wrote:
> What you propose makes the most sense to me, use a std::vector for bases, but
a
> map with a Compare that orders on getLocStart(), or something. Not a very
> expensive node comparison nor will the underlying tree come out the worse for
> it.

New patch uploaded that does this.

[Nico, 2016-01-29 18:08:07]

lgtm

[hans, 2016-01-29 18:08:23]

The CQ bit was checked by hans@chromium.org

[Nico, 2016-01-29 18:08:33]

(update cl description before hitting cq)

[hans, 2016-01-29 18:08:50]

The CQ bit was unchecked by hans@chromium.org

[hans, 2016-01-29 18:09:32]

Description was changed from

==========
blink_gc_plugin: Make RecordInfo::Get{Fields,Bases} return deterministic order

Tests are failing on the Windows bots because diagnostics get emitted in
non-deterministic order. This is a follow-up to
https://codereview.chromium.org/1645613003/ which fixed one instance, but
there were several more.

This should fix it for real by storing the fields and bases in std::vector
instead of std::map.

BUG=581782
==========

to

==========
blink_gc_plugin: Make RecordInfo::Get{Fields,Bases} return deterministic order

Tests are failing on the Windows bots because diagnostics get emitted in
non-deterministic order. This is a follow-up to
https://codereview.chromium.org/1645613003/ which fixed one instance, but
there were several more.

This should fix it for real by storing bases in a std::vector and using a
SourceLocation-based comparator for the fields map.

BUG=581782
==========

[hans, 2016-01-29 18:09:38]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2016-01-29 18:11:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1645763004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1645763004/20001

[sof, 2016-01-29 18:19:32]

lgtm

Do you plan to revert https://codereview.chromium.org/1645613003/ ?

[hans, 2016-01-29 18:29:20]

The CQ bit was unchecked by hans@chromium.org

[hans, 2016-01-29 18:29:46]

On 2016/01/29 18:19:32, sof wrote:
> Do you plan to revert https://codereview.chromium.org/1645613003/ ?

Yes, thanks for the reminder :-)

[commit-bot: I haz the power, 2016-01-29 18:43:20]

Description was changed from

==========
blink_gc_plugin: Make RecordInfo::Get{Fields,Bases} return deterministic order

Tests are failing on the Windows bots because diagnostics get emitted in
non-deterministic order. This is a follow-up to
https://codereview.chromium.org/1645613003/ which fixed one instance, but
there were several more.

This should fix it for real by storing bases in a std::vector and using a
SourceLocation-based comparator for the fields map.

BUG=581782
==========

to

==========
blink_gc_plugin: Make RecordInfo::Get{Fields,Bases} return deterministic order

Tests are failing on the Windows bots because diagnostics get emitted in
non-deterministic order. This is a follow-up to
https://codereview.chromium.org/1645613003/ which fixed one instance, but
there were several more.

This should fix it for real by storing bases in a std::vector and using a
SourceLocation-based comparator for the fields map.

BUG=581782
R=sigbjornf@opera.com, thakis@chromium.org

Committed: https://crrev.com/8734904e17cdd88103ffce6f9da1473f48e24b45
Cr-Commit-Position: refs/heads/master@{#372387}
==========

[commit-bot: I haz the power, 2016-01-29 18:43:21]

Patchset 3 (id:??) landed as
https://crrev.com/8734904e17cdd88103ffce6f9da1473f48e24b45
Cr-Commit-Position: refs/heads/master@{#372387}

[hans, 2016-01-29 18:44:23]

Description was changed from

==========
blink_gc_plugin: Make RecordInfo::Get{Fields,Bases} return deterministic order

Tests are failing on the Windows bots because diagnostics get emitted in
non-deterministic order. This is a follow-up to
https://codereview.chromium.org/1645613003/ which fixed one instance, but
there were several more.

This should fix it for real by storing bases in a std::vector and using a
SourceLocation-based comparator for the fields map.

BUG=581782
R=sigbjornf@opera.com, thakis@chromium.org

Committed: https://crrev.com/8734904e17cdd88103ffce6f9da1473f48e24b45
Cr-Commit-Position: refs/heads/master@{#372387}
==========

to

==========
blink_gc_plugin: Make RecordInfo::Get{Fields,Bases} return deterministic order

Tests are failing on the Windows bots because diagnostics get emitted in
non-deterministic order. This is a follow-up to
https://codereview.chromium.org/1645613003/ which fixed one instance, but
there were several more.

This should fix it for real by storing bases in a std::vector and using a
SourceLocation-based comparator for the fields map.

BUG=581782
R=sigbjornf@opera.com, thakis@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/8734904e17cdd88103ffce6f9da1...
==========

[hans, 2016-01-29 18:44:24]

Committed patchset #3 (id:40001) manually as
8734904e17cdd88103ffce6f9da1473f48e24b45 (presubmit successful).

[sof, 2016-02-06 08:10:22]

On 2016/01/29 18:29:46, hans wrote:
> On 2016/01/29 18:19:32, sof wrote:
> > Do you plan to revert https://codereview.chromium.org/1645613003/ ?
> 
> Yes, thanks for the reminder :-)

You're welcome... ;)