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

RESOLVED FIXED

Status

Tree Management Graveyard
TBPL
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: emorley, Assigned: RyanVM)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
B2G Arm emulator now runs reftest in 10 chunks. Resultant order is:
R1  R10  R2  R3  R4....
(Reporter)

Comment 1

4 years ago
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     }
(Assignee)

Updated

4 years ago
Assignee: nobody → ryanvm
(Assignee)

Comment 2

4 years ago
Created attachment 8379101 [details] [diff] [review]
proper chunk handling

Attaching this as a checkpoint. It works :)

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

Comment 3

4 years ago
Created attachment 8379122 [details] [diff] [review]
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.
Attachment #8379101 - Attachment is obsolete: true
Attachment #8379122 - Flags: review?(emorley)
(Reporter)

Comment 4

4 years ago
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]
Attachment #8379122 - Flags: review?(emorley) → review+
(Assignee)

Comment 5

4 years ago
(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
(Assignee)

Updated

4 years ago
Depends on: 976169
(Assignee)

Comment 6

4 years ago
In production :)
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Updated

4 years ago
Depends on: 976668
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.