[chromium-dev] Re: PSA: do not include X_messages.h in other headers
Cool! Thanks so much. I'm going to write a presubmit check for that. On Thu, Aug 20, 2009 at 11:12, John Abd-El-Malek j...@chromium.org wrote: Including files like render_messages.h and automation_messages.h from other header files is unnecessary and slows down the build (adds about ~100K lines of headers to each cc file). Last time I removed all these occurrences, it improved the build time by 15%. Looks like a few more crept in now, so I'm removing them. Please be careful not to reintroduce this, and look out for this in code reviews (yes, it would be great to have an automated way of catching this, patches welcome). --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: do not include X_messages.h in other headers
Great! Please try to add this to an existing check, or do it in a way that doesn't involve the files being read once for each presubmit check, as the presubmit step is already too slow. On Thu, Aug 20, 2009 at 11:16 AM, Paweł Hajdan Jr. phajdan...@chromium.orgwrote: Cool! Thanks so much. I'm going to write a presubmit check for that. On Thu, Aug 20, 2009 at 11:12, John Abd-El-Malek j...@chromium.org wrote: Including files like render_messages.h and automation_messages.h from other header files is unnecessary and slows down the build (adds about ~100K lines of headers to each cc file). Last time I removed all these occurrences, it improved the build time by 15%. Looks like a few more crept in now, so I'm removing them. Please be careful not to reintroduce this, and look out for this in code reviews (yes, it would be great to have an automated way of catching this, patches welcome). --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: do not include X_messages.h in other headers
Are you positive it's the per-file presubmit checks slowing things down? If so, maybe the presubmit stuff needs to be re-factored? Right now, it does each presubmit check one by one (and each check might read in the files). If it were changed to go file by file (reading fully into memory, running all the per-file pre-submit checks at once) it miight be faster. That said, it would surprise me if this was adding more than a second or two to the time. I bet most of it is waiting on other servers and such. On Thu, Aug 20, 2009 at 11:20 AM, John Abd-El-Malek j...@chromium.orgwrote: Great! Please try to add this to an existing check, or do it in a way that doesn't involve the files being read once for each presubmit check, as the presubmit step is already too slow. On Thu, Aug 20, 2009 at 11:16 AM, Paweł Hajdan Jr. phajdan...@chromium.org wrote: Cool! Thanks so much. I'm going to write a presubmit check for that. On Thu, Aug 20, 2009 at 11:12, John Abd-El-Malek j...@chromium.orgwrote: Including files like render_messages.h and automation_messages.h from other header files is unnecessary and slows down the build (adds about ~100K lines of headers to each cc file). Last time I removed all these occurrences, it improved the build time by 15%. Looks like a few more crept in now, so I'm removing them. Please be careful not to reintroduce this, and look out for this in code reviews (yes, it would be great to have an automated way of catching this, patches welcome). --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: do not include X_messages.h in other headers
The commit checks is bound to 2x appengine latency (hint hint) since it parses try job results registered on rietveld and looks up chromium-status to know if the tree is open. presubmit_support.py still reads the whole file. It's *supposed* to only load the new lines from the diff. I just assumed that would be done once it gets slow enough, you know, I didn't want to do an early optimization :D M-A On Thu, Aug 20, 2009 at 2:33 PM, Jeremy Orlowjor...@chromium.org wrote: Are you positive it's the per-file presubmit checks slowing things down? If so, maybe the presubmit stuff needs to be re-factored? Right now, it does each presubmit check one by one (and each check might read in the files). If it were changed to go file by file (reading fully into memory, running all the per-file pre-submit checks at once) it miight be faster. That said, it would surprise me if this was adding more than a second or two to the time. I bet most of it is waiting on other servers and such. On Thu, Aug 20, 2009 at 11:20 AM, John Abd-El-Malek j...@chromium.org wrote: Great! Please try to add this to an existing check, or do it in a way that doesn't involve the files being read once for each presubmit check, as the presubmit step is already too slow. On Thu, Aug 20, 2009 at 11:16 AM, Paweł Hajdan Jr. phajdan...@chromium.org wrote: Cool! Thanks so much. I'm going to write a presubmit check for that. On Thu, Aug 20, 2009 at 11:12, John Abd-El-Malek j...@chromium.org wrote: Including files like render_messages.h and automation_messages.h from other header files is unnecessary and slows down the build (adds about ~100K lines of headers to each cc file). Last time I removed all these occurrences, it improved the build time by 15%. Looks like a few more crept in now, so I'm removing them. Please be careful not to reintroduce this, and look out for this in code reviews (yes, it would be great to have an automated way of catching this, patches welcome). --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: do not include X_messages.h in other headers
On Thu, Aug 20, 2009 at 11:40 AM, Marc-Antoine Ruel mar...@chromium.orgwrote: The commit checks is bound to 2x appengine latency (hint hint) since it parses try job results registered on rietveld and looks up chromium-status to know if the tree is open. I wasn't talking about commit check, just upload (although of course it's better to make both faster). presubmit_support.py still reads the whole file. It's *supposed* to only load the new lines from the diff. I just assumed that would be done once it gets slow enough, you know, I didn't want to do an early optimization :D I think whether it loads the whole file or just a small part won't make a big difference. I suspect it's all the file operations that happen on a change with lots of files. M-A On Thu, Aug 20, 2009 at 2:33 PM, Jeremy Orlowjor...@chromium.org wrote: Are you positive it's the per-file presubmit checks slowing things down? If so, maybe the presubmit stuff needs to be re-factored? Right now, it does each presubmit check one by one (and each check might read in the files). If it were changed to go file by file (reading fully into memory, running all the per-file pre-submit checks at once) it miight be faster. That said, it would surprise me if this was adding more than a second or two to the time. I bet most of it is waiting on other servers and such. On Thu, Aug 20, 2009 at 11:20 AM, John Abd-El-Malek j...@chromium.org wrote: Great! Please try to add this to an existing check, or do it in a way that doesn't involve the files being read once for each presubmit check, as the presubmit step is already too slow. On Thu, Aug 20, 2009 at 11:16 AM, Paweł Hajdan Jr. phajdan...@chromium.org wrote: Cool! Thanks so much. I'm going to write a presubmit check for that. On Thu, Aug 20, 2009 at 11:12, John Abd-El-Malek j...@chromium.org wrote: Including files like render_messages.h and automation_messages.h from other header files is unnecessary and slows down the build (adds about ~100K lines of headers to each cc file). Last time I removed all these occurrences, it improved the build time by 15%. Looks like a few more crept in now, so I'm removing them. Please be careful not to reintroduce this, and look out for this in code reviews (yes, it would be great to have an automated way of catching this, patches welcome). --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: do not include X_messages.h in other headers
On Thu, Aug 20, 2009 at 11:33 AM, Jeremy Orlow jor...@chromium.org wrote: Are you positive it's the per-file presubmit checks slowing things down? If so, maybe the presubmit stuff needs to be re-factored? Right now, it does each presubmit check one by one (and each check might read in the files). If it were changed to go file by file (reading fully into memory, running all the per-file pre-submit checks at once) it miight be faster. That said, it would surprise me if this was adding more than a second or two to the time. I bet most of it is waiting on other servers and such. This gives me an idea: I'll add the time it takes to run presubmit checks to the output, so we can see how long it's taking. On Thu, Aug 20, 2009 at 11:20 AM, John Abd-El-Malek j...@chromium.orgwrote: Great! Please try to add this to an existing check, or do it in a way that doesn't involve the files being read once for each presubmit check, as the presubmit step is already too slow. On Thu, Aug 20, 2009 at 11:16 AM, Paweł Hajdan Jr. phajdan...@chromium.org wrote: Cool! Thanks so much. I'm going to write a presubmit check for that. On Thu, Aug 20, 2009 at 11:12, John Abd-El-Malek j...@chromium.orgwrote: Including files like render_messages.h and automation_messages.h from other header files is unnecessary and slows down the build (adds about ~100K lines of headers to each cc file). Last time I removed all these occurrences, it improved the build time by 15%. Looks like a few more crept in now, so I'm removing them. Please be careful not to reintroduce this, and look out for this in code reviews (yes, it would be great to have an automated way of catching this, patches welcome). --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
