In place of a full hero element api, we should prototype marking up our tp6 cached top sites to determine if it would be a suitable stand in for completing the hero element timing performance measures as part of the pageload metrics initiative we started earlier in 2017. The Hero Element Timing specs calls for allowing web developers to mark up their pages identifying the hero element as such: <div elementtiming=”foobar” class=”...” > We could replicate this in the cached pages and then modify talos to look for this elements visibility status via an InteractionObserver. Upon callback from the IO, mark the hero element timing result. Scope for this prototype is limited to one of the top sites to prove the idea, on one platform/config/etc., and then we'll expand to cover all of tp6 if successful.

Do we have any estimates how much using IntersectionObserver affects performance?

Daniel, do you know how much does IntersectionObserver affect performance? If the answer is any non-trivial amount, it may be hard to use for measuring page load.

(No, I don't know offhand how our IntersectionObserver impl affects perf -- haven't looked at it much yet.)

No need to review for now rwood. I've pushed it on reviewboard to make my life easier with my testing with try+windows :)

The current patch, which will be reviewed by Robert, is a first version that is collecting a timer on the first IntersectionObserver event when 100% of the marked image is visible on the viewport. It uses the google page from tp6 and adds a hero marker on obama image on the top right (wikipedia entry), which could be considered as the most important element of the page. This is an arbitrary choice we can tweak of course. I have added a tp6-hero suite in Talos to play with this, and refactored some of the pageload code to make it easier to extend.

A local run of the original tp6-google currently in production

And for comparison, a local run with :tarek's patch applied, tp6-google using the hero element. Numbers are very close :)

Just for completeness, I believe this is the actual hero element tag that :tarek added to one of the images in the tp6_google mitmproxy playback archive: elementtiming\="hero" from: <a class="bia uh_rl" href="https://www.google.ca/imgres?imgurl=http://abovethelaw.com/wp-content/uploads/2015/04/barack-obama.jpg&amp;imgrefurl=http://abovethelaw.com/barack-obama/&amp;h=2284&amp;w=3000&amp;tbnid=WRMEZ_yOiDYozM:&amp;tbnh=152&amp;tbnw=200&amp;usg=__TzXmJwa64JWqmcoNY2RoEUjQY3E=&amp;vet=1&amp;docid=DOwLSGIy-heegM&amp;itg=1&amp;hl=en&amp;sa=X&amp;ved=0ahUKEwjjmLfE_anUAhVCLmMKHe4eBgEQ_B0IgQEwDw" id="WRMEZ_yOiDYozM:" role="link" style="background:rgb(128,92,64);height:152px;margin-left:0px;margin-right:0px;margin-top:0px;width:200px" tag="bia" tabindex="0" jsaction="fire.ivg_o" data-ved="0ahUKEwjjmLfE_anUAhVCLmMKHe4eBgEQ_B0IgQEwDw"><g-img class="iuth"><img id="uid_4" elementtiming\="hero" src="data:... (:tarek please correct me if I'm wrong, thanks)

This is really cool and exciting! Thanks :tarek. I was able to apply it and run it locally on OSX (attached some json above) but then ran into some issues with talos hanging on additional runs (not clear if it's related). So I need more time to review this a bit closer. In the mean time, a few questions (mainly for :jmaher and :sphilp): Is the intent here to actually land this? Or is it just to prototype that it works? If we want to land it, will it be a direct switch-over to hero element, or will we want to run it in parallel? And will we land only tp6_google or wait and add the other sites and land them all together? If/when we want to land this, even just to run in parallel, we will need to do lots of runs on try of all pageloader tests and compare on perfherder - since this is a core change to pageloader (i.e. content handlers injection etc) and we want to ensure it doesn't effect other test numbers inadvertently. Btw, :tarek, there's another change currently in the works to enable multi-window support in talos (Bug 1409002). Unfortunately these two patches will conflict, just so you are aware in case that lands first (sorry to throw a wrench in).

Ah, talos hanging was an unrelated issue with my local setup. Will continue with this next week!

> Btw, :tarek, there's another change currently in the works to enable multi-window support in talos (Bug 1409002). Unfortunately these two patches will conflict, just so you are aware in case that lands first (sorry to throw a wrench in). No problem, I will adapt my patch in this case, under your guidance. As far as the pageloader changes are concerned, I agree we need to make sure the change is verified for regressions since it modifies how the page loader works. Whatever we end up doing with hero, I think it would be beneficial for Talos to include the core made in the pageloader because the current code is a bit fragile with string-based javascript injection. If you'd like, I could split the patch in two and have a first patch doing the small refactoring then a second one introducing hero. For whether we want to keep/replace the exisiting timer, I don't think the intent is to remove the existing page loader technique because windows.performance.* is not going anywhere. We've discussed this with Stuart Friday and agreed that a great solution would be to have the ability to record several timestamps per page load (the existing one, a hero one, etc) and plot how a page is being loaded step-by-step. That's a deeper change I would guess, but would offer more insights. For the time being, I think the intent is mostly to have a proof of concept that using the elementiming attribute when it's present will give us a more accurate measurement on when the page can start to be read by the end user. If it's the case, then we should add it in Talos as a secondary way to measure page loads in parallel of the existing tp6 measure. I guess the decision should be made with a few numbers that shows that we have a finer measurement with an elementtiming attribute. To recap, this is the timeline I would like to propose. 1. small refactoring in pageloader to use plain .js files everywhere 2. include the hero alternative in a new suite 3. verify that the hero measurement in meaningful 4. verify that the changes are not impacting talos exisiting tests 5. add tp6-hero in Talos 6. ... 7. decide wheter we want to refactor talos so one page load generates an array of timestamps as opposed to a single one we have one week until the all-hands. If there's an agreement in this timeline ^, I think a good goal would be to have 1. 2. done this week before the all-hands, then 3. to 7. could be discussed/done at the all-hands?

overall this sounds good to me.

I think the goal is to see what this does- if it works well, we can add it as a parallel test. If we find that this is more meaningful over time than regular tp6, then we replace regular tp6. I think :tarek's #7 idea is probably the most useful for helping reduce duplication in the long run.

Excellent, thanks for the feedback guys, and I agree on your proposal in comment 19 Tarek!

Thanks Robert. Should I split the patch in two parts as well?

btw, the extra \ in elementtiming\="hero" was fixed in the latest .mp file I've uploaded

I've ran all talos tests on most platform in this run https://treeherder.mozilla.org/#/jobs?repo=try&revision=60cff7a151630206dcf3359527668a05dd9d4d46 Is there a simple automated way to assess from there that the patch does not slow down things?

(In reply to Tarek Ziadé (:tarek) from comment #24) > Thanks Robert. Should I split the patch in two parts as well? Yes please I think it would be best to land the refactor separate. > btw, the extra \ in elementtiming\="hero" was fixed in the latest .mp file I've uploaded Awesome thanks for that! > Is there a simple automated way to assess from there that the patch does not slow down things? So for both of your patches (especially the refactor) I'd suggest running all of the talos tests on the 3 platforms, with at least 5 retriggers of each test. Then we can do a compare in perfherder and see how it looks. If you like, point me to a try run of each of the 2 patches when they're ready and I can do the retriggers/perfherder compare and get the numbers. Thanks!

Comment on attachment 8933228 [details] Bug 1418368 - Add a metrics for hero elementtiming - Just resetting the r flag until the patch is split/ready for review again

The refactored patch is about ready. but I am not 100% sure on how you want me to surface the option I've added in tp6_google. Should it be a global option ? or just within the test class. I've added here and there but I am not sure :) thx

(In reply to Tarek Ziadé (:tarek) from comment #32) > The refactored patch is about ready. but I am not 100% sure on how you want > me to surface the option I've added in tp6_google. > Should it be a global option ? or just within the test class. I've added > here and there but I am not sure :) > > thx Thanks Tarek I'll have a look now!

Comment on attachment 8933228 [details] Bug 1418368 - Add a metrics for hero elementtiming - https://reviewboard.mozilla.org/r/204174/#review212322 Thanks Tarek this is looking really good! Just a couple of items to follow up on. Also, please update the talos test_config unit test to test for the new 'tphero' setting [1], just add to the test similar to how 'fnbpaint' is tested. Feel free to file a bug though and add the unit test update in a future bug if you prefer. [1] https://searchfox.org/mozilla-central/source/testing/talos/talos/unittests/test_config.py ::: testing/talos/talos/cmdline.py:81 (Diff revision 12) > add_arg("--mozAfterPaint", action='store_true', dest="tpmozafterpaint", > help="wait for MozAfterPaint event before recording the time") > add_arg("--firstPaint", action='store_true', dest="firstpaint", > help="Also report the first paint value in supported tests") > + add_arg("--useHero", action='store_true', dest="tphero", > + help="use Hero elementtimer to record the time") Nit: add space in 'elementtimer' ::: testing/talos/talos/pageloader/chrome/lh_hero.js:13 (Diff revision 12) > + var el = content.window.document.querySelector("[elementtiming]"); > + if (el) { > + function callback(entries, observer) { > + entries.forEach(entry => { > + sendAsyncMessage("PageLoader:LoadEvent", > + {"fnbpaint": content.window.performance.now()}); I know that using 'fnbpaint' here just works because of the current code in plLoadHandlerMessage (which in turn calls loadHandler with parameter fnbpaint). I find it confusing though since we're not actually measuring fnbpaint here, we're measuring hero element time by grabbing performance.now. I think it would be best just to refactor loadHandler please / rename the param to something other than fnbpaint such that it either receives fnbpaint or tphero (or other future values). Then here use 'tphero' instead of 'fnbpaint'. Thanks! :) ::: testing/talos/talos/test.py:108 (Diff revision 12) > 'xperf_providers', > 'xperf_user_providers', > 'xperf_stackwalk', > 'tpmozafterpaint', > 'fnbpaint', > 'profile', also need to add 'tphero' in the list of keys too please in case we want to use hero in startup tests in the future ::: testing/talos/talos/test.py:243 (Diff revision 12) > tpmanifest = None # test manifest > tpcycles = 1 # number of time to run each page > cycles = None > timeout = None > keys = ['tpmanifest', 'tpcycles', 'tppagecycles', 'tprender', 'tpchrome', > 'tpmozafterpaint', 'fnbpaint', 'tploadnocache', 'firstpaint', 'userready', need to add 'tphero' to the list of PageloaderTest keys too please ::: testing/talos/talos/test.py:887 (Diff revision 12) > class tp6_google(QuantumPageloadTest): > """ > Quantum Pageload Test - Google > """ > tpmanifest = '${talos}/tests/quantum_pageload/quantum_pageload_google.manifest' > + tphero = False should be True

Comment on attachment 8933228 [details] Bug 1418368 - Add a metrics for hero elementtiming - https://reviewboard.mozilla.org/r/204174/#review212322 Thanks for the review. I'll add a follow-up bug - and also run again all talos tests on try to make sure we're still looking good

Comment on attachment 8933228 [details] Bug 1418368 - Add a metrics for hero elementtiming - https://reviewboard.mozilla.org/r/204174/#review213332 Awesome thanks Tarek! R+ (on condition of tp6 green on try; we added some jobs to to your try push)

:ping, do you want to land this tarek?

oh no, I thought I did before I left. Yea, let me know if I can push it now?

(In reply to Tarek Ziadé (:tarek) from comment #40) > oh no, I thought I did before I left. Yea, let me know if I can push it now? If you could please do a new try run first (not sure if it needs a rebase) just to be sure, thanks Tarek!

Pushed by tziade@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/780c013a773a Add a metrics for hero elementtiming - r=rwood