You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Darren Shepherd <da...@gmail.com> on 2013/10/04 20:52:32 UTC
Re: Review Request 12228: static resource compression
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12228/#review26690
-----------------------------------------------------------
I would prefer not to do this. From a technical perspective the code uses getRealPath and Files which are not compatible across all serlvet containers and this implementation forces a lot of file system stat() calls, but that isn't my real complaint. This change adds yet another step to the already long ACS build and I don't think it address the root issue. The root issue, if one was to really care about performance is that the js is not cachable or consolidated. ts=$now is added to every js file, meaning it must be downloaded every time. Additionally to load the login screen there are 66 requests for JS files. Even if you compress 2mb to 800k, on a slow connection it won't matter that much because you are going to get killed regardless by the round trip latency of making 66 requests.
- Darren Shepherd
On Sept. 24, 2013, 6:57 a.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12228/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2013, 6:57 a.m.)
>
>
> Review request for cloudstack, Brian Federle and Prasanna Santhanam.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CloudStack at first use downloads some 3.5 MB of css and javascript to the client. With a weak internet connection, this might take a long time. With gzip compression content can be compressed to 850 KB.
>
> This version of the patch uses a custom plugin to compress static resources, so that no dynamic compression is needed at runtime. When the static resource servlet notices that there is gzipped version of the resource and the client accepts gzipped content, then it is going to send the gziped version, while still respects http caching.
>
>
> Diffs
> -----
>
> client/WEB-INF/web.xml e5c05d3
> client/pom.xml 119c96e
> server/src/com/cloud/servlet/StaticResourceServlet.java PRE-CREATION
> server/test/com/cloud/servlet/StaticResourceServletTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/12228/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 12228: wip: static resource compression
Posted by Laszlo Hornyak <la...@gmail.com>.
> On Oct. 4, 2013, 6:52 p.m., Darren Shepherd wrote:
> > I would prefer not to do this. From a technical perspective the code uses getRealPath and Files which are not compatible across all serlvet containers and this implementation forces a lot of file system stat() calls, but that isn't my real complaint. This change adds yet another step to the already long ACS build and I don't think it address the root issue. The root issue, if one was to really care about performance is that the js is not cachable or consolidated. ts=$now is added to every js file, meaning it must be downloaded every time. Additionally to load the login screen there are 66 requests for JS files. Even if you compress 2mb to 800k, on a slow connection it won't matter that much because you are going to get killed regardless by the round trip latency of making 66 requests.
Hi Darren,
I share your concerns about the build process and I do not want to further increase that. What I had in my mind is an optional step (possibly in a profile) acivated only for making releases and by default not in a developement environment. Not that much to save 2 mb of bandwidth, but to save the time the user have to wait.
The difficulties with the non cacheable js files is indeed a problem, and I believe that should be addressed separately. (I do not quite understand yet what is that for)
Also, the dynamic informations can be compressed if it makes sense, e.g. over a given size.
I added the wip prefix to this to make it clear that this patch is "preview" version and more discussion is needed.
- Laszlo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12228/#review26690
-----------------------------------------------------------
On Oct. 4, 2013, 8:19 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12228/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 8:19 p.m.)
>
>
> Review request for cloudstack, Brian Federle and Prasanna Santhanam.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CloudStack at first use downloads some 3.5 MB of css and javascript to the client. With a weak internet connection, this might take a long time. With gzip compression content can be compressed to 850 KB.
>
> This version of the patch uses a custom plugin to compress static resources, so that no dynamic compression is needed at runtime. When the static resource servlet notices that there is gzipped version of the resource and the client accepts gzipped content, then it is going to send the gziped version, while still respects http caching.
>
>
> Diffs
> -----
>
> client/WEB-INF/web.xml e5c05d3
> client/pom.xml 119c96e
> server/src/com/cloud/servlet/StaticResourceServlet.java PRE-CREATION
> server/test/com/cloud/servlet/StaticResourceServletTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/12228/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 12228: wip: static resource compression
Posted by Laszlo Hornyak <la...@gmail.com>.
> On Oct. 4, 2013, 6:52 p.m., Darren Shepherd wrote:
> > I would prefer not to do this. From a technical perspective the code uses getRealPath and Files which are not compatible across all serlvet containers and this implementation forces a lot of file system stat() calls, but that isn't my real complaint. This change adds yet another step to the already long ACS build and I don't think it address the root issue. The root issue, if one was to really care about performance is that the js is not cachable or consolidated. ts=$now is added to every js file, meaning it must be downloaded every time. Additionally to load the login screen there are 66 requests for JS files. Even if you compress 2mb to 800k, on a slow connection it won't matter that much because you are going to get killed regardless by the round trip latency of making 66 requests.
>
> Laszlo Hornyak wrote:
> Hi Darren,
>
> I share your concerns about the build process and I do not want to further increase that. What I had in my mind is an optional step (possibly in a profile) acivated only for making releases and by default not in a developement environment. Not that much to save 2 mb of bandwidth, but to save the time the user have to wait.
>
> The difficulties with the non cacheable js files is indeed a problem, and I believe that should be addressed separately. (I do not quite understand yet what is that for)
>
> Also, the dynamic informations can be compressed if it makes sense, e.g. over a given size.
>
> I added the wip prefix to this to make it clear that this patch is "preview" version and more discussion is needed.
>
> Brian Federle wrote:
> re: caching of JS files, I believe the reason ts=$now is appended to the JS files was because of issues during upgrade testing, where the old JS files would not be cleared from the browser cache. If we can consolidate into 1 JS file then we don't need that functionality anymore, as it was more a problem due to the large # of scripts being loaded at once.
what was wrong with the http cache control headers?
- Laszlo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12228/#review26690
-----------------------------------------------------------
On Oct. 4, 2013, 8:19 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12228/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 8:19 p.m.)
>
>
> Review request for cloudstack, Brian Federle and Prasanna Santhanam.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CloudStack at first use downloads some 3.5 MB of css and javascript to the client. With a weak internet connection, this might take a long time. With gzip compression content can be compressed to 850 KB.
>
> This version of the patch uses a custom plugin to compress static resources, so that no dynamic compression is needed at runtime. When the static resource servlet notices that there is gzipped version of the resource and the client accepts gzipped content, then it is going to send the gziped version, while still respects http caching.
>
>
> Diffs
> -----
>
> client/WEB-INF/web.xml e5c05d3
> client/pom.xml 119c96e
> server/src/com/cloud/servlet/StaticResourceServlet.java PRE-CREATION
> server/test/com/cloud/servlet/StaticResourceServletTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/12228/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Laszlo Hornyak
>
>
Re: Review Request 12228: wip: static resource compression
Posted by Brian Federle <br...@citrix.com>.
> On Oct. 4, 2013, 6:52 p.m., Darren Shepherd wrote:
> > I would prefer not to do this. From a technical perspective the code uses getRealPath and Files which are not compatible across all serlvet containers and this implementation forces a lot of file system stat() calls, but that isn't my real complaint. This change adds yet another step to the already long ACS build and I don't think it address the root issue. The root issue, if one was to really care about performance is that the js is not cachable or consolidated. ts=$now is added to every js file, meaning it must be downloaded every time. Additionally to load the login screen there are 66 requests for JS files. Even if you compress 2mb to 800k, on a slow connection it won't matter that much because you are going to get killed regardless by the round trip latency of making 66 requests.
>
> Laszlo Hornyak wrote:
> Hi Darren,
>
> I share your concerns about the build process and I do not want to further increase that. What I had in my mind is an optional step (possibly in a profile) acivated only for making releases and by default not in a developement environment. Not that much to save 2 mb of bandwidth, but to save the time the user have to wait.
>
> The difficulties with the non cacheable js files is indeed a problem, and I believe that should be addressed separately. (I do not quite understand yet what is that for)
>
> Also, the dynamic informations can be compressed if it makes sense, e.g. over a given size.
>
> I added the wip prefix to this to make it clear that this patch is "preview" version and more discussion is needed.
re: caching of JS files, I believe the reason ts=$now is appended to the JS files was because of issues during upgrade testing, where the old JS files would not be cleared from the browser cache. If we can consolidate into 1 JS file then we don't need that functionality anymore, as it was more a problem due to the large # of scripts being loaded at once.
- Brian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12228/#review26690
-----------------------------------------------------------
On Oct. 4, 2013, 8:19 p.m., Laszlo Hornyak wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12228/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2013, 8:19 p.m.)
>
>
> Review request for cloudstack, Brian Federle and Prasanna Santhanam.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> CloudStack at first use downloads some 3.5 MB of css and javascript to the client. With a weak internet connection, this might take a long time. With gzip compression content can be compressed to 850 KB.
>
> This version of the patch uses a custom plugin to compress static resources, so that no dynamic compression is needed at runtime. When the static resource servlet notices that there is gzipped version of the resource and the client accepts gzipped content, then it is going to send the gziped version, while still respects http caching.
>
>
> Diffs
> -----
>
> client/WEB-INF/web.xml e5c05d3
> client/pom.xml 119c96e
> server/src/com/cloud/servlet/StaticResourceServlet.java PRE-CREATION
> server/test/com/cloud/servlet/StaticResourceServletTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/12228/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Laszlo Hornyak
>
>