mozilla :: #devtools

13 Sep 2017
09:10ntimjdescottes: ping
09:11jdescottesntim: pong
09:12ntimjdescottes: hi! I think setting a min-height had an issue, I can't remember which though
09:12ntimUnfortunately, I'm sick, so I don't have time to try again
09:13jdescottesntim: ok. I just would like to get the bug moving :) This issue has been mentioned on blog comments, twitter etc... so it would really be great to have it in 57
09:14jdescottesI couldn't see an issue when testing yesterday, I might try again later if I have time
09:14ntimjdescottes: yep, heard many people wanting this as well on reddit
09:15ntimThere might be some edge cases?
11:16Honzanchevobbe: ping
11:19nchevobbeHonza: pong
11:20Honzanchevobbe: to run reps test I need just `npm test` in devtools-reps?
11:20nchevobbeYes
11:20Honzanchevobbe: I am seeing 60 failed
11:21Honzaoh maybe yarn install ...
11:21nchevobbeYeah, maybe
11:21nchevobbe60 is a lot of failures :)
11:21HonzaI guess all of them :-)
11:29Honzanchevobbe: ok, green
11:32nchevobbeCool
11:48Honzanchevobbe: in the and I added the tests update into the same PR #663 (Null and Undefined), can you please re-review
11:49nchevobbeHonza: sure
11:49Honzanchevobbe: thanks!
12:06Honzanchevobbe: #663 says: All checks have failed
12:06Honza?
12:08nchevobbeHonza: I think there is some errors i ndevtools-core CI at the moment
12:08Honzanchevobbe: okay, I am merging then
12:10nchevobbeHonza: sounds fine
12:10nchevobbehttps://circleci.com/gh/devtools-html/devtools-core/1300?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
12:11nchevobbeHonza: this is license-check that's broken. I think jason is working on a fix on that one
12:11Honzanchevobbe: good
12:49gregtatumochameau: is there documentation on how to use damp?
12:55ochameaugregtatum: I don't think there is any doc specific to damp
12:55ochameaubut there is generic talos one:
12:55ochameauhttps://wiki.mozilla.org/Buildbot/Talos/Running
12:55ochameaurunning damp locally is as easy as running:
12:55ochameau./mach talos-test --activeTests damp
12:55ochameaubut as I said yesterday, there isn't much value in running them locally
12:56ochameauas it takes a while... and DAMP has many flaws
12:56gregtatumochameau: who are the component owners for DAMP and talos?
12:56ochameauI still have very limited trust in DAMP
12:57ochameaubut I'm working on this precise topic topic, see 1397751
12:57gregtatumok thanks
12:57ochameaugregtatum: bgrins used to write DAMP
12:57ochameaujmaher is a common name when you start talking about talos
12:58ochameauand as bgrins said in a recent devtools weekly meeting, you can run DAMP on try with:
12:58gregtatumI wasn't feeling well enough in yesterdays meeting to really chat about it, but it seems like if we have tools that aren't well supported, that are probably broken, and aren't documented we should probably either delete them, or support them better
12:58ochameau./mach try -b do -p linux64 -u none -t g2-e10s --rebuild-talos 6
12:58gregtatumi feel like damp comes up in meetings, but I have no idea how to work with or use the results
12:59ochameauI'm going for the "support them better", but most likely also going to complement that with new king of tests
12:59gregtatumi think a few people only look at the information, and the rest of the devtools ignores it, which makes it really easy to re-introduce regressions
12:59ochameaualso, if you care about performances, you don't need DAMP, just use the profiler like me or digitarald are doing
13:00gregtatumhow does DAMP work?
13:00gregtatumI don't even know
13:00ochameauas you just said, damp is only here to prevent regressions, which it does not in its current state
13:00ochameauit is run by talos on CI, talos is a python app
13:00ochameauhttp://searchfox.org/mozilla-central/source/testing/talos
13:01ochameauit involves many of the dark obscure tech firefox is implemented with
13:01ochameauold fashion xul addon, with xul overlay
13:01ochameaumessage manager, chrome to content message passing
13:01ochameauvarious kind of manifests to setup the python app to run this old fashion addon
13:02gregtatumi'm assuming it's a tool only used internally by gecko hackers
13:02ochameauthis old fashion addon is that one
13:02ochameauhttp://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools
13:02ochameauyes, I think so
13:02gregtatumso the addon probably doesn't work anymore, does it?
13:02ochameauit does
13:02ochameauwe still support old addons
13:02gregtatumhah, really?
13:02ochameaujust not on official builds
13:03ochameauon CI, that's a different story
13:03ochameauit may eventually break in future, but this addon will just be converted somehow
13:03ochameauI do have plan for Q4 to have another kind of perf test based on regular mochitest
13:03ochameaubut I prefered to focus on landing perf fixes for 57
13:04gregtatumsure, sounds reasonable
13:04ochameaugregtatum: which brings me to bug 1396633 for example ;)
13:04firebothttps://bugzil.la/1396633 NEW, nobody@mozilla.org devtools/client/framework/components/toolbox-controller.js::setCanRender is slow
13:04gregtatumochameau: haha, i'm sure there are plenty of perf improvements to the toolbar work I did
13:05ochameauthere is still a pretty decent part of setCanRender which is really misterious to me
13:05gregtatumthat work ended up being a lot harder than I imagined, and I could have used more time making it better
13:05ochameaulooks like a big react black box from the profiler call tree
13:06gregtatumyes, call tree is worthless for that type of work, the newer react has a built-in profiler with performance.measure()
13:06gregtatumthat's much more useful
13:06gregtatumwe don't support a call tree for markers yet, but you can view it in the timeline
13:06ochameaushouldn't we switch to newer react then?
13:06gregtatumyes
13:07gregtatumI think mikeratcliffe is filing a bug about it
13:07gregtatumthis will greatly help this work
13:08mikeratcliffeYup, I am logging a bug... ochameau: I have also created this:
13:08mikeratcliffehttps://irccloud.mozilla.com/file/ByFN12tK/Screen%20Shot%202017-09-13%20at%2013.28.20.png
13:08mikeratcliffeWould that be useful for you?
13:09mikeratcliffeJust adding the license stuff and then I will send it for review.
13:10ochameaumikeratcliffe: I don't know by just looking at this, last time I tried the mixins, it was way too verbose to be actionnable
13:11ochameaumikeratcliffe: but ideally, you would be the one demonstrating how such output is useful by chasing one or two perf issues ;)
13:11mikeratcliffeI have added filters to them so you can use a string or regex to limit the output to particular components.
13:11gregtatumochameau: so i've used similar logging to perform an action on one part of the page, and ensure only the things that actually changed were updated
13:11mikeratcliffeochameau: I am gonna work on perf issues now the mixins are complete.
13:13mikeratcliffeIn reality I have pretty much worked myself to death this quarter but on paper it doesn't look like it so for the rest of the month I need to blast through as many perf bugs as I can!
13:14gregtatumochameau: sorry, I'm going back to DAMP questions, can you tell me what exactly DAMP does? I know it runs on talos and does magic things, and we aren't happy with it, but other than that it's a black box
13:14gregtatumto me
13:15ochameaugregtatum: the main thing it does is that:
13:15ochameauhttp://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#285-343
13:15ochameauopen and close toolboxes for each panel
13:15ochameauand also reload the document to see the impact
13:16ochameaugregtatum: you can also run the mach command I gave, it takes quite some time to download a bunch of things, but it is very easy to run
13:18gregtatumochameau: so why does it take so long to run? It looks like it opens and tears down the different panels
13:22ochameau1/ we do that dance for each panel, we really have many!
13:23ochameau2/ it does that for two websites, simple (empty page with almost no DOM) and complicated (a copy of bild.de)
13:23ochameaugregtatum: I filled bug 1399465 to add a doc about all that
13:23firebothttps://bugzil.la/1399465 NEW, nobody@mozilla.org Document DAMP
13:24ochameaugregtatum: now running it locally is rarely useful due to variance, as we do measure time, it often varies regarding the load of your computer
13:25ochameauand our computer are often way too noisy to have stable timing
13:25ochameauthat's why we end up pushing to try, to have the whole test suite run 6 times
13:25gregtatumso that's what the magic number in the try run means
13:25ochameauand then try and perfherder are going to do average and medians to try to filter out the noise
13:26ochameauyou already saw these pages right?
13:26ochameauhttps://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=376fc6d71a3026500046bc7d65b75da89dadeaa4&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800
13:28gregtatumno i haven't
13:28gregtatumi don't know how to interpret those numbers either
13:29ochameauthe one I just gave report nothing. it is not confident about saying there is any regression nor improvement
13:29ochameaulet's compare it to this one
13:29ochameauhttps://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=0405a79a4385098205ace1531f2185ce34e79ca7&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800
13:29ochameauyou will see that all "open" tests are improved
13:30ochameauthe "confidence" column is important, it tells if the statistics algorithm (which I don't know) tells this is just within the noise ratio, or if it really looks like a real regression/improvement
13:31ochameauthis reports really relates to what I gave to you earlier, the damp addon which runs the test
13:31ochameauhttp://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#285-344
13:32ochameauyou can see on perfherder the results of all these tests
13:33gregtatumthe numbers don't have labels, what do are they? seconds?
13:33gregtatumhttps://irccloud.mozilla.com/file/FY851WSy/
13:33gregtatum*what are they
13:35ochameauit should be milliseconds
13:36ochameauit should be that time-diff
13:36ochameauhttp://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#73-76
13:36gregtatumok, next question, couldn't we collect this information through telemetry at scale, which would give us much more realistic numbers?
13:36gregtatumit would mean you can't just push a change and get immediate feedback
13:38jdescottesthrough telemetry? do you mean by polling nightly users? how would you compare timings coming from a highend desktop machine and cheap laptop?
13:38ochameauwe used to do something like that in debugger server
13:39ochameaubut I'm not sure we used the data
13:39gregtatumjdescottes: it would be in aggregate, so as the user's hardware changed over time, it would reflect in our numbers, but it would be real user timings
13:40ochameaugregtatum: the information looks appealing but not actionable
13:40gregtatumochameau: i think if you're spending time building dashboard graphs, that would make the information gathering passive (and easy to maintain) but then actionable and insightful over time
13:40ochameauyou need something to track changeset
13:40ochameauyou need something to tell you if a given patch is good or not
13:40jdescottesgregtatum: ok I guess that's nice to have a trend, but you'd get results very late compared to the time when you land a patch
13:41gregtatumjdescottes: it's be within the week for our nightly crowd
13:42ochameauand then what if the trend is bad, what is your call?
13:42ochameaureally, a per checkin is the best
13:43jdescottesI would be very careful about deciding if something is a regression or an improvement, after only a few days of data gathering
13:43gregtatumochameau: well i agree, a technical solution that gives you great data per-checkin would be really nice
13:43jdescottesI think it's an interesting data, but not useful to spot regressions/improvements for development
13:43ochameauI think it would be a good data to confirm if the per checkin solution reflect the reallity
13:44gregtatumso on a telemetry dashboard, let's say I spot a regression in inspector start up times
13:44gregtatumI wouldn't know a specific changeset that causes it
13:44gregtatumbut I know a date range
13:45gregtatumso I can checkout and build a before and after, then run the profiler to get a picture into why something is taking longer
13:45gregtatumthen I can see some kind of given change in the profilers, look into what code is causing the issue
13:45gregtatumthen I can do a blame and see all of the changes in the time range that could cause that issue
13:45ochameauat the current state of our tool, we don't need DAMP, nor anything to do that. there is performance issue to look into anytime you open the profiler with devtools open ;)
13:46gregtatumhaha, nice
13:46gregtatumso, one thing I like about telemetry is that it tells us what our real users are experiencing
13:47gregtatumwhich is oftentimes a different type of machine than we are using
13:47gregtatumit would also be something that could be very passive and not require a lot of technical knowledge and overhead to maintain
13:48jdescottesthe nightly crows is also not representative of our real users
13:48jdescottes*crowd
13:48gregtatumtrue, but the data will still ride down the train to our end users
13:49jdescottesand I'm still doubtful about the time you'd have to wait to get data you can trust :)
13:49gregtatumalthough I guess this is the whole point of go faster
13:49jdescotteswell the point is to catch regressions and act on them :)
13:50gregtatumanother concern of mine is the state of DAMP and talos
13:50solejdescottes: zer0 welp it seems like the addon sdk has been deleted already https://bugzilla.mozilla.org/show_bug.cgi?id=1371065 hehehe
13:50firebotBug 1371065 FIXED, kmaglione+bmo@mozilla.com Remove the addon SDK from Firefox
13:50jdescottesahahah
13:50soleMy browser is acting funny
13:50jdescottesnice sole
13:50zer0ah ah ah ah
13:50gregtatumsole: nice!
13:50soleWELL I'm going to do the try thing anyway just so we can ring the alarm if need be, but I'm fairly confident Kris would know better than me if things are broken ^_^
13:51ochameaugregtatum: I'm working on improving DAMP, see 1397751 and all the deps
13:51ochameauI was really sad to see this complex setup be so useless due to various dumb reasons
13:52ochameauhopefully we can address all that and have a good tool afterall
13:53gregtatumyeah, the complexity of the tool really scares me, plus the lack of documentation on it, and then the data is really hard to interpret
13:53ochameaubut complexity has a reason. ok, may be not all of it. but tracking performance on a CI is a complex story
13:54ochameaubut I do believe we can make it a decent tool with a Quarter investment on it
13:55ochameaugregtatum: do you think you will have time to look into the setCanRender patch today?
14:03gregtatumochameau: looking into it's taking 11ms to do the initial react render for the buttons, then an additional 4.8ms to update the buttonIDs
14:04gregtatumochameau: but without the react profiler markers you can't tell from the all tree what specifically it's spending time on
14:04gregtatumochameau: and 11ms for an initial react render seems reasonable to me
14:05gregtatumochameau: the idea behind setCanRender was to defer any component updates until after the state changes had time to settle down
14:22mikeratcliffeochameau: https://bugzilla.mozilla.org/show_bug.cgi?id=1399493
14:22firebotBug 1399493 NEW, mratcliffe@mozilla.com Upgrade to React 15.6.1 and include dev & prod version
14:33gregtatumochameau: oh i mis-interpreted the bug as a question on how to fix things, rather than a review
14:33gregtatumochameau: i haven't gone through my bug mail yet
14:33ochameaugregtatum: ok ok
14:34ochameaumikeratcliffe: thanks, do you know if there is any issue in upgrading or is this mostly about doing the monkey patching dance?
14:36Gijsis there a bug on file about "find" in the debugger force-scrolling back to matches making it hard/impossible to use?
14:41gregtatumochameau: usually there is upgrade work that needs to happen on each tool, and last time i believe Lin need infoed component owners who's tools needed updating, the patches were then combined and landed
14:43ochameaudoes that typically require refactoring something due to API change? or is it just a formal process to bump the version at the same time everywhere?
14:46mikeratcliffeochameau: You never know... it might just work when we have monkeypatched the files.
14:53gregtatumtypically APIs get changed over time, it's easier once you upgrade frequently because you get deprecation notices
14:53gregtatumthen it's easy to clean up deprecation notices pre-emptively
14:53gregtatummost of the work is pretty mechanical
15:02julienwI'd say that if react follows semver conventions 15.6 should be backward compatible with 15.3
15:02julienwand I think they do
15:02julienwwe can still run into surprises of course
15:34digitaraldgregtatum: to the unsupported panel point; we are putting that in for Q4
15:48tcampbellWho can I talk to about devtools/client/commandline/test/head.js?
16:03tromeytcampbell: nobody is actively working on gcli but you might as well just ask here
16:05tcampbellThanks. I need to investigate more, but roughly we have encoding issues in test cases that only accidentally worked
16:06tromeyoh, interesting
16:06tromeyI would guess that must be unintentional for gcli tests
16:07tcampbellyeah, best fix is probably to use \u escapes instead of relying on the test file being loaded as a specific encoding
16:07tcampbellbut I see big warnings that those tests live out of tree
16:07tromeyafaik they aren't maintained out of tree any more
16:08tromeythey used to be maybe? before my time.
16:08tromeyas far as I know - we can ask jwalker
16:08tcampbellah, yeah, last commit on github is years ago
16:09tromeytcampbell: but we definitely do have some tests relying on, say, style sheets being utf-8 encoded
16:09tromeyis that not ok somehow?
16:09tcampbellI'm only talking about .js loaded by loadSubscript
16:10tromeyok, thanks
16:10jwalkerso I think it's probably safe to ignore those warnings and modify in tree
16:10jwalkergcli has always needed to be able to do 2 way sync
16:11tcampbelljwalker: thanks. I'll get something working and run it by you
16:11jwalkerok, tx
16:12tcampbell(the cache used to give you back a script with a different encoding than you asked. Now it doesn't do that.. in as many cases)
16:14tromeyfunny
16:51ochameaugregtatum: I'm sorry, I have a favor to ask. I moved my patch to another bug. Would you mind stamping it again?
16:51ochameauhttps://reviewboard.mozilla.org/r/179352
16:52gregtatumochameau: done
16:52ochameaugregtatum: thanks!
16:52gregtatumochameau: thank you for improving that code :)
16:55ochameaugregtatum: half of the work was done by the profiler and perf-html ;)
17:00ochameaujryans: jdescottes: so. about lazy fronts and specs. If I apply julian suggesting right away to make "index.js" be a dependency of "protocol.js", via a index.js:getFront() and getSpec() interface, it would solve jryans's last comment about the weird feeling of importing index from protocol.js footer. Are you both fine with this proposal?
17:02jryansochameau: so who loads what? not sure i follow. index.js would require p.js, and p.js doesn't know about the index at all?
17:03ochameauyou would have that at protocol.js's head: const { getSpec, getFront } = require("devtools/shared/index");
17:03ochameauso index.js no longer known about protocol.js
17:04mstangeochameau: I'd like to talk about https://bugzilla.mozilla.org/show_bug.cgi?id=1397718 when you have a chance
17:04firebotBug 1397718 NEW, mstange@themasta.com Still missing some JS frames
17:04ochameaumstange: yes?
17:05jryansochameau: ah okay, yes, i think that approach sounds good to me!
17:05ochameaujryans: ok, great, I'll update to that.
17:05jdescottesochameau: sounds good to me too!
17:05ochameaujdescottes: \o/ I have all my reviewers on board!
17:06ochameaumstange: do you want to do a vidyo call?
17:06mstangeochameau: I prefer text actually
17:07mstangeochameau: I'd like to understand the setCanRender performance issue some more
17:07mstangewhat's the part you can't explain?
17:08mstangethe unresolved symbols in the stack are JIT frames; the JS function frames next to them are basically their symbolicated equivalent
17:08ochameaumstange: so there is a lot of frames related to react obviously
17:08mstangeyup
17:08ochameaumstange: but it is unclear why it is being slow. may be there is a mess in these react components
17:09ochameaumstange: but I was wondering if these addresses were hiding something, like the js proxy we are using
17:09ochameauI have no idea if it is really involved here, but if it does, I would be surprised if that adds an overhead
17:10ochameaumstange: ok, so everytime I see two addresses without symbol *on top* of a valid js frame, I should assume that's just a JIT story?
17:11mstangeochameau: I think that's the way it works, yes
17:11mstangeochameau: you'll see that next to the address there is no library
17:11mstangei.e. no libxul.so, for example
17:12ochameaumstange: tbh, I didn't knew that there will always be unresolved frames due to JIT
17:12mstangeso that means that this code was outside of any ranges that can be mapped to a binary / library
17:12mstangeand that usually means that this code was generated at runtime
17:13mstangeochameau: I don't know if the JS proxy should generate a call frame
17:13mstangeyou could create a testcase with a proxy and see whether it shows up
17:13ochameaumstange: ok ok, I'll try
17:13mstangethanks!
17:14ochameaumstange: otherwise I think this bug is still valid for framescripts, it looks like we still miss some
17:18ochameaumstange: wait I just looked with latest build and it looks like I got all the framescripts now
17:19ochameaumstange: I'm wondering if that only happens when I got into LUL loading, with the "mysterious" pause.
17:19ochameaumstange: beause I remember that the missing frame script frame were just after this long pause
17:20mstangeochameau: hmm, that should be unrelated
17:20mstangeLUL initialization happens synchronously on the main thread, and once it's done, everything should be happen as usual
17:21mstangeand it always happens in each new process, when the profiler is first started in that process
17:21mstangeochameau: but it's good news that the JS frames are appearing now
17:21mstangeochameau: if you see them missing again, please let me know
17:34digitaraldgregtatum ochameau nchevobbe: what are the next steps here? try it out in console?
17:43tromeyochameau: I started getting stack traces like this - http://ix.io/zUv - from protocol.js. is this familiar somehow?
18:23ochameaudigitarald: next step for?
18:24ochameautromey: wait, I haven't landed my protocol.js patch !
18:24tromeydang
18:24ochameautromey: I don't remember what it means
18:25tromeythat error? I have no idea, I didn't debug at all, I'm not even 100% sure it is new
18:25tromeyI thought you might know though
18:25ochameauI already landed lazy loading of actors... but here you have a frontend stack
18:26digitaraldochameau: ups: https://bugzilla.mozilla.org/show_bug.cgi?id=1399242
18:26firebotBug 1399242 NEW, nobody@mozilla.org Background panels cause massive overhead with React rendering
18:28ochameaudigitarald: yes, have someone write any patch. I think "any" would already introduce big wins :o
18:28ochameaudigitarald: I can look at that tomorrow, I pruned most of my patch queue today
18:29ochameautromey: random guess would be destroyed actor or null value somewhere
18:30ochameautromey: but it may be interesting, if that's a common error, to throw an explicit message. if you have STR, I can take a look, I'll propably clean up things in protocol.js sometime soon
18:31tromeyI doubt I have STR but if I see it again I will take note
18:31ochameaumstange: thanks for the detailed info in the bug. that's very helpful!
18:58tromeyI clicked "reload" in the network monitor in the browser toolbox and seemed to have broken everthing
19:09rindolfjlast: hi! How have you been?
23:55timelesshi, can someone tell me what `remote address` in the headers tab of Firefox's inspector is for?
14 Sep 2017
No messages
   
Last message: 8 days and 59 minutes ago