Code review - Issue 6826057: Lightweight assets/improve FF performancehttps://codereview.appspot.com/2012-11-09T21:27:31+00:00rietveld
Message from unknown
2012-11-05T18:04:25+00:00matthew.scotturn:md5:3f52a4648252f0fe0bf29ba2246672b7
Message from matthew.scott@canonical.com
2012-11-05T18:04:29+00:00matthew.scotturn:md5:c0ec411301c3015c883e19dc8939281c
Please take a look.
Message from gary.poster@canonical.com
2012-11-05T18:43:20+00:00gary.posterurn:md5:9c9537cb4e94809c968c6eb5c7e0d1dc
+1. Thank you.
Gary
Message from benjamin.saller@canonical.com
2012-11-05T18:57:02+00:00benjamin.sallerurn:md5:1326041f3e5e9ee2dcd35898dbb19ed6
The impl looks fine but having tested under FF the new asset doesn't seem to perform any better. This is after manually doing make appcache-force so I'm pretty sure its the current asset.
I'm not sure we should land this as it doesn't yield the benefit we hoped for.
Message from unknown
2012-11-07T22:41:26+00:00matthew.scotturn:md5:cf5c7b49bf63ff3bb334361135ea16e7
Message from matthew.scott@canonical.com
2012-11-07T22:41:28+00:00matthew.scotturn:md5:ffb534ed203df7997ee3e44018eb4ed2
Please take a look.
Message from benjamin.saller@canonical.com
2012-11-09T18:38:31+00:00benjamin.sallerurn:md5:fc4a3dbf9e68b21a623a9c4c21f96fb4
This looks good to me, I had a suggestion to test below, but either way on that one.
This doesn't fix the speed issues in FF for me, but it does improve the situation.
Thanks.
https://codereview.appspot.com/6826057/diff/3001/app/views/environment.js
File app/views/environment.js (right):
https://codereview.appspot.com/6826057/diff/3001/app/views/environment.js#newcode528
app/views/environment.js:528:
I know we talked about adding a method like this, really happy to see you added it, thanks.
https://codereview.appspot.com/6826057/diff/3001/app/views/environment.js#newcode645
app/views/environment.js:645: // .one('text').getClientRect() || {width: 0}).width + 10;
Can we specify it as d.display_name.length + 10 + 'em'? I didn't test it, but em should be font size relative if it works.
Message from gary.poster@canonical.com
2012-11-09T21:02:39+00:00gary.posterurn:md5:d32d22c68fb247100da803a7aa928d95
This makes FF usable for me both on my relatively new desktop and my 3+ year old laptop. Amazing! Thank you.
I have a few trivial comments/suggestions/requests. Approved.
Gary
https://codereview.appspot.com/6826057/diff/3001/app/modules.js
File app/modules.js (right):
https://codereview.appspot.com/6826057/diff/3001/app/modules.js#newcode14
app/modules.js:14: 'fullpath': '/juju-ui/assets/javascripts/d3.v2.js'
I guess this is good for thiago's branch, which will be minifying everything anyway, so that the debug story is nice. Is that why you did it?
https://codereview.appspot.com/6826057/diff/3001/app/views/environment.js
File app/views/environment.js (right):
https://codereview.appspot.com/6826057/diff/3001/app/views/environment.js#newcode645
app/views/environment.js:645: // .one('text').getClientRect() || {width: 0}).width + 10;
I'm -1 on checking in commented-out code to trunk; if you add an explanatory comment as to why you have left this in but commented out, I might feel better about it. :-)
Message from matthew.scott@canonical.com
2012-11-09T21:21:32+00:00matthew.scotturn:md5:4bfe6573df46b4f51a109ecb4108eedb
Thanks
https://codereview.appspot.com/6826057/diff/3001/app/modules.js
File app/modules.js (right):
https://codereview.appspot.com/6826057/diff/3001/app/modules.js#newcode14
app/modules.js:14: 'fullpath': '/juju-ui/assets/javascripts/d3.v2.js'
Left over from profiling; back to minified.
On 2012/11/09 21:02:40, gary.poster wrote:
> I guess this is good for thiago's branch, which will be minifying everything
> anyway, so that the debug story is nice. Is that why you did it?
https://codereview.appspot.com/6826057/diff/3001/app/views/environment.js
File app/views/environment.js (right):
https://codereview.appspot.com/6826057/diff/3001/app/views/environment.js#newcode645
app/views/environment.js:645: // .one('text').getClientRect() || {width: 0}).width + 10;
Unfortunately not, as we use this attribute on line 649, expecting it to be pixels, as we don't know how wide an em is.
On 2012/11/09 18:38:31, benjamin.saller wrote:
> Can we specify it as d.display_name.length + 10 + 'em'? I didn't test it, but em
> should be font size relative if it works.
https://codereview.appspot.com/6826057/diff/3001/app/views/environment.js#newcode645
app/views/environment.js:645: // .one('text').getClientRect() || {width: 0}).width + 10;
Ooops! Done!
On 2012/11/09 21:02:40, gary.poster wrote:
> I'm -1 on checking in commented-out code to trunk; if you add an explanatory
> comment as to why you have left this in but commented out, I might feel better
> about it. :-)
Message from unknown
2012-11-09T21:25:21+00:00matthew.scotturn:md5:b8ae1a80c5ff7d4147cbd480164c057d
Message from matthew.scott@canonical.com
2012-11-09T21:27:31+00:00matthew.scotturn:md5:4dc5f5c332289f0c9cab90ca0f6b257c
*** Submitted:
Lightweight assets/improve FF performance
Some lighter weight assets for service modules were implemented, via Matt C. Additionally, profiling showed that a lot of slowness for FireFox was caused by FF internals being slow (namely <node>.setAttribute() and <node>.getClientRects()). While we can't fix those being slow, we can reduce the number of times they're used. This is done by modifying attributes on only relevant relation lines when a service is dragged (rather than redrawing or modifying all lines). This results in fewer calls to both of those internals, and speeds up animation in FireFox.
R=gary.poster, benjamin.saller
CC=
https://codereview.appspot.com/6826057