Closed Bug 1263270 Opened 4 years ago Closed 4 years ago

Sort census reports by smallest node ID counted, rather than number of nodes counted

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
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
Attachment #8739551 - Flags: review?(jimb) → review+
(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 :)
Attachment #8739551 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d5748680699f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.