mozilla :: #activity-stream

13 Sep 2017
13:26ursulaHey everyone I'm at the doctor's this morning, will be in before noon. I'm working on the about home tests, should probably be done today
13:37k88hudsonursula: awesome sounds good!
13:55nanjMardak: thanks for fixing the talos test harness ;)
13:55nanjMardak: so we have to wait your patch to land first, right?
14:14ahillierMardak: r1cky: Hey so I'm doing Fairly straightforward, but it does mean there's at least three highlights fetch calls hitting Places very quickly on first start (from postInit, topsites being updated, and new tab load) and then broadcasting actions. Just wanted to ask if that's a performance issue?
14:20Mardakahillier: theoretically the postInit one will be skipped and wait for topsites ?
14:20Mardakif checking getState().TopSites.initialized
14:21ursulak88hudson: I'll probably need your help debugging these search tests :(
14:21Mardakand NEW_TAB_LOAD updates only if not full or after 15 minutes.. although on performance test profiles, most likely highlights will be empty...
14:21ahillierMardak: I've got fetchHighlights await-ing this
14:22k88hudsonursula: sure thing
14:22Mardakahillier: ah. trying to avoid our previous tippytop initialized skipping? ;)
14:25Mardaki've been thinking if we should have a cache in topsites and highlights..
14:25ahillierFor the places queries?
14:25Mardake.g., for pinning, there's been some desire to have it "refresh" to recalculate pinned positions and deduping
14:25Mardakbut it doesn't actually need to refetch from places
14:26ahillierAh that makes sense
14:26Mardakyeah caching the result from places/NewTabUtils
14:27Mardaknot sure if the cache timer should be the same 15 minute and on force refresh, we just reset that lastUpdated time
14:27Mardakcould even stick in the images into that cache :p ;)
14:28r1cky+1 for caching. actually i think it already is cached in `this.highlights`?
14:28Mardakoh true
14:28r1ckyor maybe that isnt the raw results
14:28Mardakah yeah that's the final results
14:28ahillierMardak: yeah that would be great, and means we could get rid of the imageCache thing
14:31Mardakahillier: ah but probably not for 57? we should be able to take your patch and push to try to check talos
14:32ahillierMardak: yeah sounds good, will finish up the tests and push
14:43Mardakfiled the caching issue
14:49Mardakr1cky: hrmm at least from my browsing patterns (maybe just a lot of github issues and not much news) but the numItems * 5 multiplier isn't enough for me resulting in just 3 highlights ;)
14:49Mardaksolution: we should have fewer activity stream issues!
14:49r1ckyhahaha true
14:50r1ckypagination *gulp*
14:51r1ckyright now i have 9 highlights fwiw
14:52r1ckyMardak: i assume we can tell from telemetry how many people are in your same bucket?
14:53Mardakmmmm maybe? emtwo are we getting telemetry on how many highlights are shown?
14:54r1ckywe need to now how deep is the user's history + how many highlights are show?
14:54r1ckyneed coffee
14:55emtwoMardak: we used to have a field called "highlights_size" in test pilot but it doesn't seem we've ported that over to the system addon
14:56Mardakemtwo: ok hrmm do we get something similar for the number of pocket stories shown? highlights is implemented with the same section behavior as stories
14:57Mardakr1cky: here's a rough count (from browser console) that doesn't dedupe against topsites. top 50 highlights would show 5 without deduping but only 3 after deduped
14:57Mardaks = new Set(); NewTabUtils.activityStreamLinks.getHighlights({numItems: 50}).then(r => r.forEach(l => s.add(new URL(l.url).hostname))).then(() => console.log(s.size, s))
14:58emtwoMardak: the best we have right now is position of clicks which doesn't really say much about how many were shown...
14:59emtwoMardak: we also used to have impression counts per pocket story, which again doesnt say how many pocket stories were shown either
15:00Mardakr1cky: with 100, it would probably see 9 highlights, so maybe we should change *5 to *10... although i have 86 github highlights in the last 100 pages that have description+image :p so.. maybe i'm just odd ;)
15:20r1ckyMardak: does it affect the performance of the query?
15:20r1cky86 github pages out of last 100 does seem extreme
15:22Mardakbrb. managed to burn soup o.O fire alarm beeping from 2 rooms away from the kitchen!
15:23r1ckyoh boy
15:32tspurwaynow im hungry
15:37streaminatorr1cky: Hey! Someone requested a review:
15:45streaminatorahillier: Hey, your patch was approved!
15:51streaminatorcsadilek: Hey, your patch was approved!
15:51streaminatorcsadilek: Hey, your patch was approved!
16:10r1ckyMardak: so, playing around with the about:home stuff for auto focus... I notice that if I open a new window, for example, there is telemetry sent for visibility_event_rcvd_ts and topsites_first_painted_ts. BUT, when I startup the browser, it only sends the topsites_first_painted_ts. Not sure if that's expected or not
16:45Mardakr1cky: aha. i think k88hudson worked around this similar issue. content sends to main a message before ActivityStreamMessageChannel is set up
16:46k88hudsonMardak: yep, I added some code to retry if we receive an INIT event later
16:46k88hudsonr1cky: it could be modified to queue more events possibly
16:46Mardaki suppose dmose ^^
16:49streaminatorr1cky: Hey! Someone requested a review:
16:49Mardakk88hudson: although your queue seems to be from the chrome side
16:52streaminatork88hudson: Hey, your patch was approved!
16:53k88hudsonMardak: r1cky: there's no queue right now, just a specific code path for reattempting
16:54Mardakr1cky: filed
17:51Mardakursula: you're everywhere :p
17:52ursulaMardak: is this what it would be like if i was part of triplets??
18:00Mardakursula: ;) getScreenshots test timing out :p send one of the triplets to fix it!
18:01Mardakk88hudson: r1cky's patch just waits for SAVE_SESSION_PERF_DATA action with topsites painted on chrome side to focus the browser
18:01Mardaki suppose more correct would be to add a new action FOCUS_ME_PLZ
18:01k88hudsonor you could wait for visibility
18:02Mardakk88hudson: from the chrome side?
18:02Mardakwe need to focus 2 things: the browser and the search box in that browser
18:13streaminatorr1cky: Hey, your patch was approved!
18:13streaminatorr1cky: Hey, your patch was approved!
18:15ursulaMardak: kate found the problem with the failing search tests, so now we can keep them on and still land about home! can you review the patch? basically i split out browser_aboutHome.js into 3 tests, 2 of which turn AS off, and then browser_aboutHome_search.js keeps AS on and just fixes the query selectors to look for our search box
18:17ursulait also fixes hehe
18:17Mardakr1cky: yeah try dispatching a new action from where you call input.focus(). i think it should behave the same if you do it from onInputMount
18:19Mardakursula: nice ;)
18:20r1ckyMardak: yeah, good idea on doing it onInputMount
18:23streaminatorfmarier: Hey! Someone requested a review:
19:01streaminatorMardak: Hey! Someone requested a review:
19:12streaminatorahillier: Hey, your reviewer requested some changes:
19:15ursulaif anyone needs to dump work, i'm done all of my bugs for 57 P1 so i can take something
19:16Mardakwhat?! not jumping straight to fun P2s? :p
19:16ursula43 open P2s :o the possibilities are endless
19:17ursulaso much to choose from
19:19Mardakursula: fyi there's a new Polish column in the project ;)
19:20ursulaMardak: you're full of fun ideas today aren't you hahahah
19:21Mardakoh i only pointed it out because you put some stuff into Chores. but picking up Polish bugs might make some sense :p
19:22ursulaoh my bad!
19:22Mardak.. because polishing stuff isn't a chore? ;)
19:23ursuladepends what you consider a chore!
19:23ursulahmmm just noticed that if you have a pinned top site and you clear the history it doesn't get rid of the pinned site
19:24ursulaby design?
19:24Mardaki believe that's the same behavior as tiles
19:28ursular1cky: i'm looking at, is this actually by design? this: does that on purpose
19:29Mardakursula: well design just said to have it editable so maybe designs changed
19:29Mardakbut good to figure out the history
19:29r1ckyursula: if "by design" means k88hudson and I decided it, yes
19:30Mardakohh hrmm.. does that affect empty placeholder stuff?
19:30r1ckybut i guess it makes sense to show edit. dont know if that causes any side effects
19:33ursulait doesn't look like it does
19:37Mardakursula: oh. never knew you could give querySelectors an array
19:37ursulak88hudson: said the same thing!
19:38k88hudsonMardak: ursula: yeah it's weird, it works in Chrome as well
19:38Mardakwell clearly i only select the one thing i care about and i select it correctly!! >:P
19:38ursulaclearly you've never had two versions of about:home
19:45ursulaMardak: i'm actually pushing an update to the patch, i need to put ignoreAllUncaughtExceptions at the top of the tests
19:48ursula ok good to go
20:02streaminatorahillier: Hey, your patch was approved!
20:04ursulaMardak: yeah i was thinking about the request timeout thing... according to bug 1258717 the one that might be the longest would be search
20:06ursulahe seemed to think that splitting into 3 tests would be enough to remove the request
20:06ursulabut i can split the search one further if you don't want to take any chances
20:06Mardakwell.. math says something will exceed the timeout, no? ;)
20:07ursulathis is true
20:07Mardakunless someone was being conservative and picked 4 instead of 3 or 2 :p
20:08ursulaso, let me make sure i understand it in the first place... the request was because the file itself had tests that could go on for longer than the expected time for a mochitest
20:09ursulaor is it for *all* tests in that directory?
20:09Mardakfor that file
20:10ursulaand it's not dependent on the number of add_tasks exactly
20:10ursulabut rather just how long it takes in general
20:11Mardaknod. that's why splitting up the file would help avoid needing the requestLonger
20:11Mardaker. sorry. it is dependent on number of add_tasks because more tasks means potentially more slower tests
20:12Mardake.g., if each test took 1 minute and the timeout is 5 minutes, if you had 6 tests, it would need to request a longer timeout
20:12ursulabut i guess it's hard to know *which* one of the ones i split out needs longer
20:13ursulamike seems to think it's the search ones
20:13ursulafrom that bug
20:14ursulathough snippets has the most add_tasks
20:15Mardakursula: looks like you got your try in just in time. it's broken/not accepting new pushes right now "remote: waiting for lock on working directory of /repo/hg/mozilla/try held by process '29707' on host ''
20:16Mardaki was trying to do this to get more results faster ;) try: -b do -p linux,linux64,macosx64,win32,win64 -u mochitest-browser-chrome-1,mochitest-browser-chrome-e10s-1,mochitest-e10s-browser-chrome-1 --try-test-paths browser-chrome:browser/base/content/test/about --artifact --rebuild 20
20:16ursulaoh shit
20:16ursulaactually that would have been better than mine
20:19ursulashould i split it into 5? search/snippets_1 search/snippets_2?
20:23streaminatorr1cky: Hey! Someone requested a review:
20:25Mardakursula: oh my try went through now. i guess we'll see if anything times out
20:25Mardakor.. we can just autoland and wait for something to time out......
20:26ursulaMardak: let's wait on the try push
20:26ursulashouldn't be that long
20:28streaminatorursula: Hey, your patch was approved!
20:46Mardakursula: ha. here's what was causing try to hang
20:46ursulaMardak: uh oh ......
20:47Mardakoooh leakkkk
20:50Mardakursula: so either some tests running after that one happened to clean up the leak or enough time passed for it to be cleaned up..
20:50Mardakhrm and non e10s
20:51Mardakursula: linux64 opt FAIL | browser/base/content/test/about/browser_aboutHome_search.js | Test timed out [log]
20:51ursulahm so the search ones may need more time?
21:01streaminatorr1cky: Hey, your patch was approved!
21:02nanjdmose: your patch is working
21:02nanjno ping with "unknown" page yet, which i think it's a good thing :)
21:03Mardakunless.. there's a bug and it fails to send when unknown! ;)
21:04nanjheh, i trust dmose
21:04ursulai'm trying with requestLongerTimeout(2) on all of them
21:04ursulasee what happens
21:05Mardakursula: looks like it's only _search
21:06Mardakleak shows up on linux,64,windows. mac is slow but seems likely it would leak there too
21:06ursulai figured i would just do it to be safe and then process of elimination that shit until the leak goes awa
21:10dmosenanj: hah, thanks!
21:12Mardakursula: the request longer shouldnt affect the leak. i'll try building debug locally
21:14dmosenanj: to be fair, i'd expect some unknown pings, since there is code to send some intentionally
21:16dmoseMardak: btw, if we ever do need to create an event asking for focus, it needs to be called I_CAN_HAZ_FOKUS_PLZ
21:19streaminatorncloudioj: Hey! Someone requested a review:
21:22ursulaMardak: do you think the leak could have something to do with switching from clearUserPref to specialpowers ?
21:24streaminatorfmarier: Hey! Someone requested a review:
21:27ursulai'm tempted to just do dmose's way and just turn activity stream off for the tests and then fix them in for AS in parallel
21:27ursulawhat do you think Mardak?
21:30Mardakursula: well the leaking window seems to clearly point to the first test of aboutHome.js so maybe that one can be disabled. maybe just move that one test into its own file and skip-if=debug
21:33Mardakursula: oh and i see _search is still timing out with 2 with your latest try
21:33dmoseMardak: ursula: mike deboer added add_task().skip() in July
21:33Mardakdmose: well theoretically we can keep test coverage on opt builds
21:34dmoseah, true
21:34dmosefwiw,!topic/ has the details if it's useful iln the future
21:41streaminatorcsadilek: Hey, your patch was approved!
21:42ursulaalternatively .......
21:43Mardakursula: looks to leak on my linux64 vm ./mach mochitest --disable-e10s
21:43Mardakreturning early from that imitate test avoids leak
21:44ursulait's bizarre because that test should have activity stream off
21:44nanjdmose: oh, that query only touched today's data. You think we should have some unknowns today?
21:45dmosenanj: i would have expected a few, yes. but i haven't analyzed the code paths in any detail.
21:50nanjdmose: i can dig deeper once you figure that out. I'll send a bad state ping if we're dropping unknowns in the system :)
21:50dmosenanj: sounds good :-)
21:50Mardakursula: it might leak even without splitting the tests. it leaks go away when i set firefox.js aboutHome.enabled false
21:53ursulai'm happy to keep debugging this but i'm wondering if we should just land turning aboutHome.enabled for this test today
22:00ursulaMardak: i've got to head out, what do you think we should do?
22:01Mardakursula: looks to pass locally. i'll reopen the split test bug and you can attach the just disable test
22:01ursulawhen you say "attach the just disable test" you mean the patch i posted up there ^
22:02Mardakyeah the latest try
22:02Mardak+function promiseDisableOnboardingToursAndActivityStream() {
22:02ursulaok will do
22:07ursulaMardak: ok pushed
22:08ursulashould i land it?
22:09Mardakursula: i requested autoland already
22:09ursulatoo fast for me ;)
22:10ursulathanks Mardak!
22:23andreiodmose: not having a common telemetry perf recording component came back to bite me. I had to write twice the number of tests.
22:34Mardakdmose: are you removing the unnecessary
14 Sep 2017
No messages
Last message: 7 days and 5 hours ago