talos tsvg fails to record data properly when a manifest has a mix of own timing and pageloader timing

RESOLVED FIXED

Status

Testing
Talos
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
tsvg has a problem where gearflowers.svg only reports 1 data point when it should be reporting 3, likewise hixie007.xml is reporting 5 data points when it should be reporting 3.  

NOISE: __start_tp_report
NOISE: _x_x_mozilla_page_load
NOISE: _x_x_mozilla_page_load_details
NOISE: |i|pagename|runs|
NOISE: |0;gearflowers.svg;2537
NOISE: |1;composite-scale.svg;681;541;548
NOISE: |2;composite-scale-opacity.svg;1142;1156;1125
NOISE: |3;composite-scale-rotate.svg;2042;2042;2019
NOISE: |4;composite-scale-rotate-opacity.svg;2435;2431;2459
NOISE: |5;hixie-001.xml;14818;15054;15172
NOISE: |6;hixie-002.xml;14828;15008;15205
NOISE: |7;hixie-003.xml;305253;325679;331091
NOISE: |8;hixie-004.xml;5703;5830;5669
NOISE: |9;hixie-005.xml;25175;25323;25153
NOISE: |10;hixie-006.xml;30446;30432;30371
NOISE: |11;hixie-007.xml;6448;2256;6739;2320;6720
NOISE: __end_tp_report

The problem is in http://hg.mozilla.org/build/talos/file/f2aec5d0a58c/talos/pageloader/chrome/pageloader.js#l426 where we record the time, but don't clear all the global state variables, in this case recordedName.  So the next test that runs will record the results for the previous test.
(Assignee)

Comment 1

5 years ago
Created attachment 691287 [details] [diff] [review]
reset the recordedName for tests that do own timing (1.0)

this only seems to affect tests which have mixed timing modes in a manifest and svg.manifest is the only manifest we have that has mixed modes.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #691287 - Flags: review?(jhammel)
The same symptom seems to also occur with tp4m

https://datazilla.allizom.org/talos/testdata/raw/Mozilla-Inbound/a8233178dd72?product=Fennec&test_name=Talos%20tp4m

the replicate count varies between 1, 2, 3 depending on what page was run.

Comment 3

5 years ago
Comment on attachment 691287 [details] [diff] [review]
reset the recordedName for tests that do own timing (1.0)

good detective work on this one!  wfm
Attachment #691287 - Flags: review?(jhammel) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 691490 [details] [diff] [review]
solve the >1 cycle problem (2.0)

the original patch fixed a loophole in mixed mode manifests, this patch fixes a hole in numCycles>1 (on desktop we just do one cycle and use pagecycles instead).

This is the original patch + the new one combined!
Attachment #691287 - Attachment is obsolete: true
Attachment #691490 - Flags: review?(jhammel)

Comment 5

5 years ago
Comment on attachment 691490 [details] [diff] [review]
solve the >1 cycle problem (2.0)

r+ nice :)
Attachment #691490 - Flags: review?(jhammel) → review+
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/build/talos/rev/1eab02b9475e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.