1497758 - getCurrentPageShortName returns 'benchmarks' for displaylist_mutate

Status: RESOLVED FIXED

(Testing :: Talos, enhancement, P3)

Description

[Matt Woodrow (:mattwoodrow)]

It looks like this code has baked in assumptions about the path structure, and displaylist_mutate is nested one deeper.

This causes us to initialize the profiler with the exact same file name for all subtests, which clobber each other and we only get the output for the last one.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

I assume we just need to turn off the pagename format:
https://searchfox.org/mozilla-central/source/testing/talos/talos/test.py#806

just add that to the displaylist_mutate test:
https://searchfox.org/mozilla-central/source/testing/talos/talos/test.py#952


:bebe, want to pick this up soon?

Comment 2

[Florin Strugariu [:Bebe]]

Attachment: Bug 1497758 getCurrentPageShortName returns 'benchmarks' for displaylist_mutate r? igoldan jmaher

Comment 3

[Ionuț Goldan [:igoldan]]

I pushed this on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80e39276d79a8285996042dd61d75f684fe20d7f

Comment 4

[Florin Strugariu [:Bebe]]

:jmaher please take a look at my comments in the code review
https://phabricator.services.mozilla.com/D8363


:mattwoodrow do you have a example on how you initialize the profiler?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:bebe, thanks for following up here, I left some comments in the review- I think we are on the wrong track (my fault for making a bad assumption) and getting access to a profile will help us solve this the right way

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Florin Strugariu [:Bebe] from comment #4)
> :mattwoodrow do you have a example on how you initialize the profiler?

I think he filed this for profiles on try server the display list mutate talos test.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

we generate profiles for every push to mozilla-central, here you can see the g4 job which runs displaylist_mutate has profiles for linux/osx/windows:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&tier=1%2C2%2C3&searchStr=talos%2Cg4%2Cprofiling&selectedJob=205791892&revision=9079bbe837184ed183b133a374753865b6768bc4

they all load for me.  If there is an edge case to fix, I would like to fix it.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #7)
> we generate profiles for every push to mozilla-central, here you can see the
> g4 job which runs displaylist_mutate has profiles for linux/osx/windows:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> central&tier=1%2C2%2C3&searchStr=talos%2Cg4%2Cprofiling&selectedJob=205791892
> &revision=9079bbe837184ed183b133a374753865b6768bc4
> 
> they all load for me.  If there is an edge case to fix, I would like to fix
> it.

Apologies, missed this ni?.

They do indeed load, but when loaded they give the option of 'benchmarks_pagecycle_1' and 'benchmarks_pagecycle_2' (and cycle_0.profile a sub-option for both).

Displaylist_mutate has 3 subtests [1], and with two cycles, I'd expect 6 profiles within the zip, not 2.

The code for figuring out the subtest name (getCurrentPageShortName) has a bug where it is computing 'benchmarks' as the subtest name in this case, resulting in all 3 subtests writing to the same output profile file, and overwriting each other.

The end result is just the profile from the last subtest, and stored under the 'benchmarks' name, not the actual subtest name.



[1] https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/layout/displaylist_mutate.manifest

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

thanks :mattwoodrow, this is actionable.

:bebe, after you get the new pagesets, this would be useful to look into.

Comment 10

[Florin Strugariu [:Bebe]]

leaving this as new as I'm not working on it

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

How important is this to be fixed? It stalled for more than half a year now.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

I would assume this isn't of high importance. I think the frustration is when there is a regression in this test the profiler would end up not being very helpful- so low importance is probably due to not needing the profiler for this test as often.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Yeah, we haven't had to investigate regressions in this test for a while, so it hasn't mattered.

Comment 14

[Brad Werth [:bradwerth]]

Attachment: Bug 1497758: Make talos getCurrentPageShortName provide unique names for long test urls.

Comment 15

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00f55def23f3ba5f61e710ecbb39c4fc817860e9

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a286cedfc5c0
Make talos getCurrentPageShortName provide unique names for long test urls. r=perftest-reviewers,sparky

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a286cedfc5c0