Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2381093002:
Move GetNewTabCSS() caching off the startup path. (Closed)
|
Created:
4 years, 2 months ago by Alexei Svitkine (slow) Modified:
4 years, 2 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove GetNewTabCSS() caching off the startup path.
This function appears to take about a mean of 35ms during
startup according to UMA sampling profiler data. Also, the
current NTP doesn't need this CSS - since this is related to
the old local NTP, which currently only powers chrome://apps.
This also saves memory used by the generated CSS until the
first time it's used. The downside is there will be a delay
when loading this css - but given this codepath is only used
by chrome://apps page, this seems like a reasonable trade-off.
Note: The CSS will still be cached after the first request, since
NTPResourceCache caches it and we fetch it from there.
No intended functional changes. Expecting to see a 35ms
startup improvement in UMA when this lands.
BUG=651848
Committed: https://crrev.com/4417714b419462d698d03a790293818c6cecd81f
Cr-Commit-Position: refs/heads/master@{#422578}
Patch Set 1 #Patch Set 2 : Remove unneeded include. #
Total comments: 4
Patch Set 3 : Address comments. #Patch Set 4 : Fix test. #
Messages
Total messages: 38 (19 generated)
Description was changed from ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to sampling profiler. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=todo ========== to ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to sampling profiler. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=todo ==========
Description was changed from ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to sampling profiler. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=todo ========== to ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to sampling profiler. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=todo ==========
Description was changed from ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to sampling profiler. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=todo ========== to ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to sampling profiler. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=todo ==========
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to sampling profiler. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=todo ========== to ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to sampling profiler. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=651848 ==========
asvitkine@chromium.org changed reviewers: + sky@chromium.org
Description was changed from ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to sampling profiler. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=651848 ========== to ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to UMA sampling profiler data. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=651848 ==========
I'm pretty sure this was done to make the new tab appear quicker. I suspect this would break that. None-the-less can you get a more local reviewer please.
asvitkine@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam sky: See CL description. This codepath is not used by the current NTP it's only used by chrome://apps.
dbeam: Please review per chrome/browser/ui/webui owners.
Excellent! Thanks for clarifying (and I should have read the description more carefully). -Scott On Fri, Sep 30, 2016 at 9:27 AM, <asvitkine@chromium.org> wrote: > +dbeam > > sky: See CL description. This codepath is not used by the current NTP it's > only > used by chrome://apps. > > https://codereview.chromium.org/2381093002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dbeam: friendly ping Or let me know if you prefer that I find a different reviewer.
doesn't this end up regenerated the incognito or guest new tab css every time a page is shown?
regenerating*
dbeam@chromium.org changed reviewers: + estade@chromium.org
+estade@ as he may be more familiar with this moving the creation off the startup path is fine but may still want to keep a cached copy
I believe the CSS is still cached by NTPResourceCache.
On 2016/10/03 16:25:40, Alexei Svitkine (slow) wrote: > I believe the CSS is still cached by NTPResourceCache. See this code: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/ntp/ntp_resource... It will create it first time and then keep it as a member as |new_tab_css_|. This CL makes us fetch it from NTPResourceCache - so it will still be cached. I'll expand the CL description to mention this as well.
Description was changed from ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to UMA sampling profiler data. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=651848 ========== to ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to UMA sampling profiler data. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. Note: The CSS will still be cached after the first request, since NTPResourceCache caches it and we fetch it from there. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=651848 ==========
awesome! lgtm https://codereview.chromium.org/2381093002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/theme_source.cc (right): https://codereview.chromium.org/2381093002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:65: base::RefCountedMemory* GetNewTabCSSOnUIThread(Profile* profile) { nit: style guide says NewTabCssOnUiThread https://codereview.chromium.org/2381093002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:80: original_profile_(profile->GetOriginalProfile()) { why not just inline profile_->GetOriginalProfile where it's needed?
lgtm (agree with estade@'s comment assuming it's functionally equivalent)
Thanks - comments addressed. https://codereview.chromium.org/2381093002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/theme_source.cc (right): https://codereview.chromium.org/2381093002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:65: base::RefCountedMemory* GetNewTabCSSOnUIThread(Profile* profile) { On 2016/10/03 20:16:59, Evan Stade wrote: > nit: style guide says NewTabCssOnUiThread Done - changed the other function in this file too for consistency. https://codereview.chromium.org/2381093002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:80: original_profile_(profile->GetOriginalProfile()) { On 2016/10/03 20:16:59, Evan Stade wrote: > why not just inline profile_->GetOriginalProfile where it's needed? Done.
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2381093002/#ps100001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2381093002/#ps120001 (title: "Fix test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to UMA sampling profiler data. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. Note: The CSS will still be cached after the first request, since NTPResourceCache caches it and we fetch it from there. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=651848 ========== to ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to UMA sampling profiler data. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. Note: The CSS will still be cached after the first request, since NTPResourceCache caches it and we fetch it from there. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=651848 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to UMA sampling profiler data. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. Note: The CSS will still be cached after the first request, since NTPResourceCache caches it and we fetch it from there. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=651848 ========== to ========== Move GetNewTabCSS() caching off the startup path. This function appears to take about a mean of 35ms during startup according to UMA sampling profiler data. Also, the current NTP doesn't need this CSS - since this is related to the old local NTP, which currently only powers chrome://apps. This also saves memory used by the generated CSS until the first time it's used. The downside is there will be a delay when loading this css - but given this codepath is only used by chrome://apps page, this seems like a reasonable trade-off. Note: The CSS will still be cached after the first request, since NTPResourceCache caches it and we fetch it from there. No intended functional changes. Expecting to see a 35ms startup improvement in UMA when this lands. BUG=651848 Committed: https://crrev.com/4417714b419462d698d03a790293818c6cecd81f Cr-Commit-Position: refs/heads/master@{#422578} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4417714b419462d698d03a790293818c6cecd81f Cr-Commit-Position: refs/heads/master@{#422578} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
