Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690263004:
Add option to GRIT to provide a resource ordering input file. (Closed)
|
Created:
3 years, 10 months ago by Alexei Svitkine (slow) Modified:
3 years, 10 months ago Reviewers:
Nico CC:
chromium-reviews, asvitkine+watch_chromium.org, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd option to GRIT to provide a resource ordering input file.
This allows optimizing Chrome's start up sequence such that the
resources that are loading during startup appear in order in the
resource .pak files and thus incur less page faults to load from
disk and a faster start up time and less memory used (due to less
pages resident in RAM).
This change adds the necessary infrastructure to support this,
but does not yet enable it for any platform. That will be done in
a separate change later.
BUG=692670
Review-Url: https://codereview.chromium.org/2690263004
Cr-Commit-Position: refs/heads/master@{#451464}
Committed: https://chromium.googlesource.com/chromium/src/+/9a84244d0aabd3d3ee775adf82dc9acb2fefac31
Patch Set 1 : With Mac resource ordering file. #Patch Set 2 : Removed Mac resource ordering file. #
Total comments: 6
Patch Set 3 : Address comments. #
Total comments: 6
Patch Set 4 : Address comments. #Patch Set 5 : Reverse dict using 1-liner-ish. #
Dependent Patchsets: Messages
Total messages: 51 (30 generated)
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Dry run: 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
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add option to GRIT to provide a resource ordering input file. This allows optimizing Chrome's start up sequence such that the resources that are loading during startup appear in order in the resource .pak files and thus incur less page faults to load from disk and a faster start up time and less memory used (due to less pages resident in RAM). BUG= ========== to ========== Add option to GRIT to provide a resource ordering input file. This allows optimizing Chrome's start up sequence such that the resources that are loading during startup appear in order in the resource .pak files and thus incur less page faults to load from disk and a faster start up time and less memory used (due to less pages resident in RAM). BUG=692670 ==========
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Description was changed from ========== Add option to GRIT to provide a resource ordering input file. This allows optimizing Chrome's start up sequence such that the resources that are loading during startup appear in order in the resource .pak files and thus incur less page faults to load from disk and a faster start up time and less memory used (due to less pages resident in RAM). BUG=692670 ========== to ========== Add option to GRIT to provide a resource ordering input file. This allows optimizing Chrome's start up sequence such that the resources that are loading during startup appear in order in the resource .pak files and thus incur less page faults to load from disk and a faster start up time and less memory used (due to less pages resident in RAM). This change adds the necessary infrastructure to support this, but does not yet enable it for any platform. That will be done in a separate change later. BUG=692670 ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
asvitkine@chromium.org changed reviewers: + thakis@chromium.org
Looks generally fine. I idly wonder if we should call this "order_file" instead of "predetermined_ids", but that's bikesheddy so I'll leave it at idle wonder :-) https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/format... File tools/grit/grit/format/rc_header.py (right): https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/format... tools/grit/grit/format/rc_header.py:80: _predetermined_tids = {} nit: python style guide says 2 blank lines between toplevels https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/format... tools/grit/grit/format/rc_header.py:126: if predetermined_ids: given the previous lien, won't predetermined_ids always be false-y? (it's always an empty dict now) https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/tool/b... File tools/grit/grit/tool/build.py (right): https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/tool/b... tools/grit/grit/tool/build.py:91: file will assigned ids normally. The file is of the format: s/will/will be/ Have you considered if missing IDs should be an error instead of automatically getting an ID assigned? Why the "silently do a suboptimal thing" decision? (There might be a good reason, just want to know it)
https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/format... File tools/grit/grit/format/rc_header.py (right): https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/format... tools/grit/grit/format/rc_header.py:80: _predetermined_tids = {} On 2017/02/17 21:44:08, Nico wrote: > nit: python style guide says 2 blank lines between toplevels Done. https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/format... tools/grit/grit/format/rc_header.py:126: if predetermined_ids: On 2017/02/17 21:44:08, Nico wrote: > given the previous lien, won't predetermined_ids always be false-y? (it's always > an empty dict now) Woops, this is meant to be predetermined_tids originally. But actually even that wasn't needed - since predetermined_tids is also always initialized to at least an empty dict. So the if wasn't needed - removed. https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/tool/b... File tools/grit/grit/tool/build.py (right): https://codereview.chromium.org/2690263004/diff/100001/tools/grit/grit/tool/b... tools/grit/grit/tool/build.py:91: file will assigned ids normally. The file is of the format: On 2017/02/17 21:44:08, Nico wrote: > s/will/will be/ Done. > > Have you considered if missing IDs should be an error instead of automatically > getting an ID assigned? Why the "silently do a suboptimal thing" decision? > (There might be a good reason, just want to know it) Yep. The order file only has resources that are loaded during startup. Without data from the wild, we don't know what an optimal order is for the other ids - so there's no point in re-assigning them to something arbitrary.
lgtm https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/format... File tools/grit/grit/format/rc_header.py (right): https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/format... tools/grit/grit/format/rc_header.py:130: predetermined_ids[id] = tid I think predetermined_ids = dict(predetermined_tids) will do the same thing as these 3 lines https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/tool/b... File tools/grit/grit/tool/build.py (right): https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/tool/b... tools/grit/grit/tool/build.py:93: RESOURCE_TWO_NAME 124 i'd mention somewhere "The motivation is to run your app's startup and make it dump the resources it uses, and then pass those via -p; that way those get small IDs helping startup time due to less paging while all other resources are unperturbed"
https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/format... File tools/grit/grit/format/rc_header.py (right): https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/format... tools/grit/grit/format/rc_header.py:130: predetermined_ids[id] = tid On 2017/02/17 22:15:42, Nico wrote: > I think > > predetermined_ids = dict(predetermined_tids) > > will do the same thing as these 3 lines This is doing a reverse, so your suggestion doesn't help. https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/tool/b... File tools/grit/grit/tool/build.py (right): https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/tool/b... tools/grit/grit/tool/build.py:93: RESOURCE_TWO_NAME 124 On 2017/02/17 22:15:42, Nico wrote: > i'd mention somewhere "The motivation is to run your app's startup and make it > dump the resources it uses, and then pass those via -p; that way those get small > IDs helping startup time due to less paging while all other resources are > unperturbed" Done. https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/tool/b... tools/grit/grit/tool/build.py:93: RESOURCE_TWO_NAME 124 On 2017/02/17 22:15:42, Nico wrote: > i'd mention somewhere "The motivation is to run your app's startup and make it > dump the resources it uses, and then pass those via -p; that way those get small > IDs helping startup time due to less paging while all other resources are > unperturbed" Done.
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2690263004/#ps140001 (title: "Address comments.")
https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/format... File tools/grit/grit/format/rc_header.py (right): https://codereview.chromium.org/2690263004/diff/120001/tools/grit/grit/format... tools/grit/grit/format/rc_header.py:130: predetermined_ids[id] = tid On 2017/02/17 22:39:25, Alexei Svitkine (slow) wrote: > On 2017/02/17 22:15:42, Nico wrote: > > I think > > > > predetermined_ids = dict(predetermined_tids) > > > > will do the same thing as these 3 lines > > This is doing a reverse, so your suggestion doesn't help. Ah, sorry. predetermined_ids = {value: key for key value in predetermined_tis}
forgot a comma:
predetermined_ids = {value: key for key, value in predetermined_tis}
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
but that pending 2 line reduction! :-P On Fri, Feb 17, 2017 at 5:41 PM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/ > codereview.chromium.org/2690263004/140001 > > > https://codereview.chromium.org/2690263004/ > -- 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.
The CQ bit was unchecked by asvitkine@chromium.org
Sure, that works. Done. Needed to wrap because you gotta do .iteritems() and so its 2 lines in the end. On Fri, Feb 17, 2017 at 5:40 PM, <thakis@chromium.org> wrote: > forgot a comma: > > predetermined_ids = {value: key for key, value in predetermined_tis} > > > https://codereview.chromium.org/2690263004/ > -- 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.
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2690263004/#ps160001 (title: "Reverse dict using 1-liner-ish.")
Is that .iteritems really needed? Huh. On Fri, Feb 17, 2017 at 5:45 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > Sure, that works. Done. > > Needed to wrap because you gotta do .iteritems() and so its 2 lines in the > end. > > On Fri, Feb 17, 2017 at 5:40 PM, <thakis@chromium.org> wrote: > >> forgot a comma: >> >> predetermined_ids = {value: key for key, value in predetermined_tis} >> >> >> https://codereview.chromium.org/2690263004/ >> > > -- 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.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Without it: ValueError: too many values to unpack On Fri, Feb 17, 2017 at 5:46 PM, Nico Weber <thakis@chromium.org> wrote: > Is that .iteritems really needed? Huh. > > On Fri, Feb 17, 2017 at 5:45 PM, Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> Sure, that works. Done. >> >> Needed to wrap because you gotta do .iteritems() and so its 2 lines in >> the end. >> >> On Fri, Feb 17, 2017 at 5:40 PM, <thakis@chromium.org> wrote: >> >>> forgot a comma: >>> >>> predetermined_ids = {value: key for key, value in predetermined_tis} >>> >>> >>> https://codereview.chromium.org/2690263004/ >>> >> >> > -- 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.
Ah. From a few seconds in the repl, I guess it's
>>> {a[k]:k for k in a}
{2: 1, 4: 3}
without .iteritems()
On Fri, Feb 17, 2017 at 5:46 PM, Alexei Svitkine <asvitkine@chromium.org>
wrote:
> Without it:
>
> ValueError: too many values to unpack
>
> On Fri, Feb 17, 2017 at 5:46 PM, Nico Weber <thakis@chromium.org> wrote:
>
>> Is that .iteritems really needed? Huh.
>>
>> On Fri, Feb 17, 2017 at 5:45 PM, Alexei Svitkine <asvitkine@chromium.org>
>> wrote:
>>
>>> Sure, that works. Done.
>>>
>>> Needed to wrap because you gotta do .iteritems() and so its 2 lines in
>>> the end.
>>>
>>> On Fri, Feb 17, 2017 at 5:40 PM, <thakis@chromium.org> wrote:
>>>
>>>> forgot a comma:
>>>>
>>>> predetermined_ids = {value: key for key, value in predetermined_tis}
>>>>
>>>>
>>>> https://codereview.chromium.org/2690263004/
>>>>
>>>
>>>
>>
>
--
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.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by asvitkine@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by asvitkine@chromium.org
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_chromeos_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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487431625735390,
"parent_rev": "8a40fe5f00e8b6c3363194f69fe9b2b4f79781b8", "commit_rev":
"9a84244d0aabd3d3ee775adf82dc9acb2fefac31"}
Message was sent while issue was closed.
Description was changed from ========== Add option to GRIT to provide a resource ordering input file. This allows optimizing Chrome's start up sequence such that the resources that are loading during startup appear in order in the resource .pak files and thus incur less page faults to load from disk and a faster start up time and less memory used (due to less pages resident in RAM). This change adds the necessary infrastructure to support this, but does not yet enable it for any platform. That will be done in a separate change later. BUG=692670 ========== to ========== Add option to GRIT to provide a resource ordering input file. This allows optimizing Chrome's start up sequence such that the resources that are loading during startup appear in order in the resource .pak files and thus incur less page faults to load from disk and a faster start up time and less memory used (due to less pages resident in RAM). This change adds the necessary infrastructure to support this, but does not yet enable it for any platform. That will be done in a separate change later. BUG=692670 Review-Url: https://codereview.chromium.org/2690263004 Cr-Commit-Position: refs/heads/master@{#451464} Committed: https://chromium.googlesource.com/chromium/src/+/9a84244d0aabd3d3ee775adf82dc... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/chromium/src/+/9a84244d0aabd3d3ee775adf82dc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
