Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710673004:
precache: Allow experiment bitsets of any size. (Closed)
Descriptionprecache: Allow experiment bitsets of any size.
Add bytes-typed field to PrecacheResourceSelection, which allows for an
arbitrarily-sized bit vector, and thus doesn't limit the number of
resources per site to 64.
BUG=696012
Review-Url: https://codereview.chromium.org/2710673004
Cr-Commit-Position: refs/heads/master@{#452921}
Committed: https://chromium.googlesource.com/chromium/src/+/b5a09c31d9b7af63c2b82194200bbb3ed9c5a878
Patch Set 1 #
Total comments: 6
Patch Set 2 : Update comments, make test bitset asymmetric, and make proto field deprecation more obvious. #
Total comments: 8
Dependent Patchsets: Messages
Total messages: 27 (12 generated)
Description was changed from ========== Allow experiment bitsets of unlimited size. Add bytes-typed field to PrecacheResourceSelection, which allows for an arbitrarily-sized bit vector, and thus doesn't limit the number of resources per site to 64. BUG= ========== to ========== precache: Allow experiment bitsets of any size. Add bytes-typed field to PrecacheResourceSelection, which allows for an arbitrarily-sized bit vector, and thus doesn't limit the number of resources per site to 64. BUG= ==========
Description was changed from ========== precache: Allow experiment bitsets of any size. Add bytes-typed field to PrecacheResourceSelection, which allows for an arbitrarily-sized bit vector, and thus doesn't limit the number of resources per site to 64. BUG= ========== to ========== precache: Allow experiment bitsets of any size. Add bytes-typed field to PrecacheResourceSelection, which allows for an arbitrarily-sized bit vector, and thus doesn't limit the number of resources per site to 64. BUG= ==========
twifkak@chromium.org changed reviewers: + jamartin@chromium.org
LGTM. https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:159: // Only one return point to ensure RVO triggers. (opt) Not needed. Besides, AFIK, it is not so much that you return in a single point but that you only return the same single variable. https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher_unittest.cc (right): https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher_unittest.cc:586: ret.set_deprecated_bitset(0b10101); Can you please change the testcase for one that it is not symmetric? Maybe 0b10011? It is indeed not symmetric in the new bitset, but still I'd like it to be explicit in both selections. https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... File components/precache/core/proto/precache.proto (right): https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:56: optional fixed64 deprecated_bitset = 1 [default = 0xFFFFFFFFFFFFFFFF]; s/deprecated_bitset/DEPRECATED_bitset/ Maybe set [deprecated = true], although the deprecation message may be annoying or prevent the submission.
https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:159: // Only one return point to ensure RVO triggers. On 2017/02/22 22:14:01, jamartin wrote: > (opt) Not needed. Besides, AFIK, it is not so much that you return in a single > point but that you only return the same single variable. Fixed the comment. (I want to head off at the pass any future temptation to define a local vector<bool> and then return it, causing an implicit construction of the Optional and therefore who knows what performance.) https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher_unittest.cc (right): https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher_unittest.cc:586: ret.set_deprecated_bitset(0b10101); On 2017/02/22 22:14:01, jamartin wrote: > Can you please change the testcase for one that it is not symmetric? > > Maybe 0b10011? > > It is indeed not symmetric in the new bitset, but still I'd like it to be > explicit in both selections. Done. Made it multibyte, too, to test the mixed ordering. https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... File components/precache/core/proto/precache.proto (right): https://codereview.chromium.org/2710673004/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:56: optional fixed64 deprecated_bitset = 1 [default = 0xFFFFFFFFFFFFFFFF]; On 2017/02/22 22:14:01, jamartin wrote: > s/deprecated_bitset/DEPRECATED_bitset/ > > Maybe set [deprecated = true], although the deprecation message may be annoying > or prevent the submission. Done.
The CQ bit was checked by twifkak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamartin@chromium.org Link to the patchset: https://codereview.chromium.org/2710673004/#ps20001 (title: "Update comments, make test bitset asymmetric, and make proto field deprecation more obvious.")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
twifkak@chromium.org changed reviewers: + bengr@chromium.org
Ben: Please review (or refer me to sclittle or rajendrant if you're swamped). I have another few such CLs I'll be sending your way. Thanks!
pasko@chromium.org changed reviewers: + pasko@chromium.org
rubberstamp ltgm (is it needed for a committer to l-g-t-m if the sender is already a committer?) A few questions about direction below. https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:133: uint32_t experiment_id) { Do I understand correctly that each server manifest maps a finch group to a bitset, and then you also experiment with different bitsets in the manifest generation pipeline? Wow. Advanced. Did you find 64 to be limiting in some specific cases or you just want to experiment with larger bitsets and see what happens? https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:159: // Only return one variable to ensure RVO triggers. what is RVO? Render View Observer?
I mean, rubberstamp lgtm
The CQ bit was checked by twifkak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'd add an issue ID, but lgtm fwiw.
Description was changed from ========== precache: Allow experiment bitsets of any size. Add bytes-typed field to PrecacheResourceSelection, which allows for an arbitrarily-sized bit vector, and thus doesn't limit the number of resources per site to 64. BUG= ========== to ========== precache: Allow experiment bitsets of any size. Add bytes-typed field to PrecacheResourceSelection, which allows for an arbitrarily-sized bit vector, and thus doesn't limit the number of resources per site to 64. BUG=696012 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487964803277480,
"parent_rev": "6cb5aaabcff899c2399082578944c9f8a88cee86", "commit_rev":
"b5a09c31d9b7af63c2b82194200bbb3ed9c5a878"}
Message was sent while issue was closed.
Description was changed from ========== precache: Allow experiment bitsets of any size. Add bytes-typed field to PrecacheResourceSelection, which allows for an arbitrarily-sized bit vector, and thus doesn't limit the number of resources per site to 64. BUG=696012 ========== to ========== precache: Allow experiment bitsets of any size. Add bytes-typed field to PrecacheResourceSelection, which allows for an arbitrarily-sized bit vector, and thus doesn't limit the number of resources per site to 64. BUG=696012 Review-Url: https://codereview.chromium.org/2710673004 Cr-Commit-Position: refs/heads/master@{#452921} Committed: https://chromium.googlesource.com/chromium/src/+/b5a09c31d9b7af63c2b82194200b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b5a09c31d9b7af63c2b82194200b...
Message was sent while issue was closed.
(It's possible that a committer-reviewer is needed only for CQ, and not `git cl land`.) https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:133: uint32_t experiment_id) { On 2017/02/24 13:42:33, pasko wrote: > Do I understand correctly that each server manifest maps a finch group to a > bitset, and then you also experiment with different bitsets in the manifest > generation pipeline? Yup. > Wow. Advanced. Thanks. Or sorry. :P > Did you find 64 to be limiting in some specific cases or you just want to > experiment with larger bitsets and see what happens? Don't know for certain, but we suspect it's an issue. 64 is the number of _unfiltered_ resources -- the more filters we experiment with, the fewer _filtered_ resources per experiment we get. So, e.g. if we experiment with filtering out short-lived or uncached resources, and 90% of resources are filtered out, then we only get 6.4 resources per manifest. In addition, with global ranking, it may be useful for some sites to have more resources while others have less. https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:159: // Only return one variable to ensure RVO triggers. On 2017/02/24 13:42:33, pasko wrote: > what is RVO? Render View Observer? Return Value Optimization. Constructs the object on the stack frame of the calling function, rather than constructing it here and then copying or moving it back.
Message was sent while issue was closed.
thank you for responses, Devin https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:133: uint32_t experiment_id) { On 2017/02/24 21:20:12, twifkak wrote: > On 2017/02/24 13:42:33, pasko wrote: > > Do I understand correctly that each server manifest maps a finch group to a > > bitset, and then you also experiment with different bitsets in the manifest > > generation pipeline? > Yup. > > > Wow. Advanced. > Thanks. Or sorry. :P > > > Did you find 64 to be limiting in some specific cases or you just want to > > experiment with larger bitsets and see what happens? > Don't know for certain, but we suspect it's an issue. 64 is the number of > _unfiltered_ resources -- the more filters we experiment with, the fewer > _filtered_ resources per experiment we get. So, e.g. if we experiment with > filtering out short-lived or uncached resources, and 90% of resources are > filtered out, then we only get 6.4 resources per manifest. > > In addition, with global ranking, it may be useful for some sites to have more > resources while others have less. Ah, thank you for background. Naive question: why not applying bitsets after filtering? Is it something historical? https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:159: // Only return one variable to ensure RVO triggers. On 2017/02/24 21:20:12, twifkak wrote: > On 2017/02/24 13:42:33, pasko wrote: > > what is RVO? Render View Observer? > > Return Value Optimization. Constructs the object on the stack frame of the > calling function, rather than constructing it here and then copying or moving it > back. Acknowledged.
Message was sent while issue was closed.
https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:133: uint32_t experiment_id) { On 2017/02/27 18:12:16, pasko wrote: > On 2017/02/24 21:20:12, twifkak wrote: > > On 2017/02/24 13:42:33, pasko wrote: > > > Do I understand correctly that each server manifest maps a finch group to a > > > bitset, and then you also experiment with different bitsets in the manifest > > > generation pipeline? > > Yup. > > > > > Wow. Advanced. > > Thanks. Or sorry. :P > > > > > Did you find 64 to be limiting in some specific cases or you just want to > > > experiment with larger bitsets and see what happens? > > Don't know for certain, but we suspect it's an issue. 64 is the number of > > _unfiltered_ resources -- the more filters we experiment with, the fewer > > _filtered_ resources per experiment we get. So, e.g. if we experiment with > > filtering out short-lived or uncached resources, and 90% of resources are > > filtered out, then we only get 6.4 resources per manifest. > > > > In addition, with global ranking, it may be useful for some sites to have more > > resources while others have less. > > Ah, thank you for background. Naive question: why not applying bitsets after > filtering? Is it something historical? Perhaps I misunderstand your question. The bitsets are the mechanism for client-side filtering. The idea is we want to play with different manifest variants; it would be possible to generate and host multiple copies of the manifests, but this is much cheaper. So let's say we want to experiment with two versions of the manifest: Variant 1: [a.js, b.js] Variant 2: [a.js, c.js] Then we'd encode that as: {resources: [a.js, b.js, c.js], variants: {1: 0x3, 2: 0x5}} Because the bitset was a fixed64, that put a hard limit on the number of elements in 'resources' at 64, regardless of how many bits are set. PS--The history of this feature is we added it to test the question "Should we precache images?" We've more recently used it to test several other what-ifs, though.
Message was sent while issue was closed.
https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2710673004/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:133: uint32_t experiment_id) { On 2017/02/27 21:46:28, twifkak wrote: > On 2017/02/27 18:12:16, pasko wrote: > > On 2017/02/24 21:20:12, twifkak wrote: > > > On 2017/02/24 13:42:33, pasko wrote: > > > > Do I understand correctly that each server manifest maps a finch group to > a > > > > bitset, and then you also experiment with different bitsets in the > manifest > > > > generation pipeline? > > > Yup. > > > > > > > Wow. Advanced. > > > Thanks. Or sorry. :P > > > > > > > Did you find 64 to be limiting in some specific cases or you just want to > > > > experiment with larger bitsets and see what happens? > > > Don't know for certain, but we suspect it's an issue. 64 is the number of > > > _unfiltered_ resources -- the more filters we experiment with, the fewer > > > _filtered_ resources per experiment we get. So, e.g. if we experiment with > > > filtering out short-lived or uncached resources, and 90% of resources are > > > filtered out, then we only get 6.4 resources per manifest. > > > > > > In addition, with global ranking, it may be useful for some sites to have > more > > > resources while others have less. > > > > Ah, thank you for background. Naive question: why not applying bitsets after > > filtering? Is it something historical? > > Perhaps I misunderstand your question. The bitsets are the mechanism for > client-side filtering. The idea is we want to play with different manifest > variants; it would be possible to generate and host multiple copies of the > manifests, but this is much cheaper. So let's say we want to experiment with two > versions of the manifest: > > Variant 1: [a.js, b.js] > Variant 2: [a.js, c.js] > > Then we'd encode that as: > > {resources: [a.js, b.js, c.js], > variants: {1: 0x3, 2: 0x5}} > > Because the bitset was a fixed64, that put a hard limit on the number of > elements in 'resources' at 64, regardless of how many bits are set. > > PS--The history of this feature is we added it to test the question "Should we > precache images?" We've more recently used it to test several other what-ifs, > though. Oh, ah. Having bitsets _for_ filtering makes a lot of sense to me. Thank you. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
