821242 - Need more intelligent sort for double digit machine names (eg 'R10')

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Ed Morley [:emorley]]

B2G Arm emulator now runs reftest in 10 chunks. Resultant order is:
R1  R10  R2  R3  R4....

Comment 1

[Ed Morley [:emorley]]

Relevant code is at:

https://hg.mozilla.org/webtools/tbpl/file/0a3def9520dd/js/Data.js#l421

   421     if (Config.groups.indexOf(group) != -1) {
   422       // Sort the jobs by type (eg reftest/crashtest), then by chunk number.
   423       // The space is so "Mochitest [0-9]" sorts before "Mochitest {Browser Chrome,Other}"
   424       var aNum = result.machine.type + ' ' + result.machine.jobPartNumber();
   425       comparator = function (b) {
   426         var bNum = b.machine.type + ' ' + b.machine.jobPartNumber();
   427         if (aNum == bNum)
   428           return compareStateTime(b);
   429 
   430         return aNum > bNum ? 1 : -1;
   431       }
   432     }

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: proper chunk handling

Attaching this as a checkpoint. It works :)

Does not include the optimization work we discussed on IRC, though.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Attachment: proper chunk handling avoiding unnecessary work in the common case

This avoids running the jobPartNumber regex on the second job in the common case where the job types are different.

Comment 4

[Ed Morley [:emorley]]

Comment on attachment 8379122 [details] [diff] [review]
proper chunk handling avoiding unnecessary work in the common case

Review of attachment 8379122 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the changes below :-)

::: js/Data.js
@@ +420,5 @@
>      var comparator = compareStateTime;
>      if (Config.groups.indexOf(group) != -1) {
>        // Sort the jobs by type (eg reftest/crashtest), then by chunk number.
> +      var aNum = parseInt(result.machine.jobPartNumber());
> +      var aType = result.machine.type;

Per IRC, can we swap these two variable declarations to be semantically less to more specific? :-)

@@ +427,5 @@
> +        if (aType != bType)
> +          return aType > bType ? 1 : -1;
> +
> +        var bNum = parseInt(b.machine.jobPartNumber());
> +

With the next comment implemented, think we can ditch the newline here :-)

@@ +432,4 @@
>          if (aNum == bNum)
>            return compareStateTime(b);
>  
>          return aNum > bNum ? 1 : -1;

Could we swap these last two return statements to make the comparisons semantically in order of least to most specific?
ie: [compare type] -> [compare number] -> [last resort: compareStateTime]

Comment 5

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #4)
> Per IRC, can we swap these two variable declarations to be semantically less
> to more specific? :-)

Whoops, I made that change locally and forgot to update the attached patch apparently :)

> With the next comment implemented, think we can ditch the newline here :-)

Done.

> Could we swap these last two return statements to make the comparisons
> semantically in order of least to most specific?
> ie: [compare type] -> [compare number] -> [last resort: compareStateTime]

Done.

https://hg.mozilla.org/webtools/tbpl/rev/aa8f3a0e2308

Comment 6

[Ryan VanderMeulen [:RyanVM]]

In production :)