Comment on attachment 8739551 [details] [diff] [review] Sort census reports by smallest node ID counted, rather than number of nodes counted Review of attachment 8739551 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks! No effect on any tests?? ::: js/public/UbiNodeCensus.h @@ +144,5 @@ > + > + bool ret = type.count(*this, mallocSizeOf, node); > + > + MOZ_ASSERT(total_ == oldTotal, > + "CountType::count should not increment total_, CountBase::count handles that"); cute ::: js/src/vm/UbiNodeCensus.cpp @@ +312,5 @@ > > > +// Comparison function for sorting hash table entries by the smallest node ID > +// they counted. The arguments are doubly indirect: they're pointers to elements > +// in an array of pointers to table entries. It'd be nice to briefly explain the rationale for this sort order. "Stable and unique, to ensure ordering of results never depends on hash table placement or sort algorithm vagaries" or whatever. @@ -874,5 @@ > bool > ByFilename::count(CountBase& countBase, mozilla::MallocSizeOf mallocSizeOf, const Node& node) > { > Count& count = static_cast<Count&>(countBase); > - count.total_++; lovely

(In reply to Jim Blandy :jimb from comment #3) > No effect on any tests?? Yeah, I was pretty surprised by this! > > +// Comparison function for sorting hash table entries by the smallest node ID > > +// they counted. The arguments are doubly indirect: they're pointers to elements > > +// in an array of pointers to table entries. > > It'd be nice to briefly explain the rationale for this sort order. "Stable > and unique, to ensure ordering of results never depends on hash table > placement or sort algorithm vagaries" or whatever. Good idea, that sounds great, so I will just lift it :)

This should fix the failure in the last try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f31561f2d401