Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(285)

Issue 2008553007: [Courgette] PagedArray: Add Iterators and Parametrize Page Size as int Template. (Closed)

Created:
4 years, 7 months ago by huangs
Modified:
4 years, 6 months ago
Reviewers:
Nico, chrisha, etiennep
CC:
chromium-reviews, wfh+watch_chromium.org, huangs+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Courgette] PagedArray: Add Iterators and Parametrize Page Size as int Template. This is a refactoring CL to enable PagedArray usage by libdivsufsort. In addition to overloading operator[], for more general usage we need need pointer-like accessors to PagedArray. To this end we implement PagedArray_const_iterator and PagedArray_const_iterator, which merely wraps a PagedArray pointer along with an index. We also add various operators needed by libdivsufsort. For optimization, '<' and '<=' operators omits pointer checks. By default PagedArray page size is 2**18 elements (1 MiB for int32_t). To enable better testing, we made (log) page size a tepmlate parameter. BUG=608885 Committed: https://crrev.com/804ed8a1e0f236f22e19abef7013e9641a3ad49d Cr-Commit-Position: refs/heads/master@{#397311}

Patch Set 1 #

Total comments: 22

Patch Set 2 : Make better use of C++11. #

Patch Set 3 : Make Clang happy. #

Patch Set 4 : Follow etiennep@'s suggestion to use single iterator class. #

Total comments: 12

Patch Set 5 : More cleanup. #

Patch Set 6 : git cl format #

Total comments: 8

Patch Set 7 : Adding more checks for Debug build only. #

Patch Set 8 : Reverting 2 versions of operator[]() to duplicate code. #

Patch Set 9 : Add operators > and >= to make Clang std::sort() happy. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+386 lines, -26 lines) Patch
M courgette/third_party/bsdiff/paged_array.h View 1 2 3 4 5 6 7 8 3 chunks +173 lines, -13 lines 0 comments Download
M courgette/third_party/bsdiff/paged_array_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +213 lines, -13 lines 0 comments Download

Messages

Total messages: 41 (15 generated)
huangs
Prerequisite for adopting libdivsufsort, PTAL. Thanks!
4 years, 7 months ago (2016-05-24 21:23:08 UTC) #2
etiennep
Great, I love iterators! Round one. https://codereview.chromium.org/2008553007/diff/1/courgette/third_party/bsdiff/paged_array.h File courgette/third_party/bsdiff/paged_array.h (right): https://codereview.chromium.org/2008553007/diff/1/courgette/third_party/bsdiff/paged_array.h#newcode41 courgette/third_party/bsdiff/paged_array.h:41: struct iterator_traits<class courgette::PagedArray_iterator<T, ...
4 years, 7 months ago (2016-05-25 14:19:02 UTC) #3
huangs
Thanks! Looked at std::vector::iterator and did it a more conventional way of having non-const iterator ...
4 years, 7 months ago (2016-05-25 19:36:42 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008553007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008553007/20001
4 years, 7 months ago (2016-05-25 19:37:26 UTC) #6
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/166312)
4 years, 7 months ago (2016-05-25 19:46:21 UTC) #8
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008553007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008553007/40001
4 years, 7 months ago (2016-05-25 21:50:31 UTC) #10
commit-bot: I haz the power
Dry run: None
4 years, 7 months ago (2016-05-25 23:08:16 UTC) #11
huangs
Updated. There were more details to be flush out, but should be good. PTAL. Thanks!
4 years, 7 months ago (2016-05-25 23:10:49 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008553007/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008553007/60001
4 years, 7 months ago (2016-05-25 23:12:22 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 7 months ago (2016-05-26 01:14:22 UTC) #16
commit-bot: I haz the power
Dry run: None
4 years, 7 months ago (2016-05-26 01:14:50 UTC) #17
etiennep
Round two! I think conversions between different iterator types is handled in a clever way. ...
4 years, 7 months ago (2016-05-26 14:03:30 UTC) #18
huangs
Updated, PTAL. https://codereview.chromium.org/2008553007/diff/60001/courgette/third_party/bsdiff/paged_array.h File courgette/third_party/bsdiff/paged_array.h (right): https://codereview.chromium.org/2008553007/diff/60001/courgette/third_party/bsdiff/paged_array.h#newcode16 courgette/third_party/bsdiff/paged_array.h:16: #include <cstddef> On 2016/05/26 14:03:29, etiennep wrote: ...
4 years, 6 months ago (2016-05-26 17:54:27 UTC) #19
etiennep
lgtm
4 years, 6 months ago (2016-05-26 18:12:21 UTC) #20
huangs
Ping chrisha@ for review, PTAL.
4 years, 6 months ago (2016-05-26 20:51:01 UTC) #21
chrisha
lgtm with nits https://codereview.chromium.org/2008553007/diff/100001/courgette/third_party/bsdiff/paged_array.h File courgette/third_party/bsdiff/paged_array.h (right): https://codereview.chromium.org/2008553007/diff/100001/courgette/third_party/bsdiff/paged_array.h#newcode30 courgette/third_party/bsdiff/paged_array.h:30: template <typename ContainerType, typename T> A ...
4 years, 6 months ago (2016-05-31 20:15:17 UTC) #22
huangs
Thanks! Updated and committing soon. Local benchmarking shows that having operator[]() use common code via ...
4 years, 6 months ago (2016-06-01 22:07:30 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008553007/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008553007/140001
4 years, 6 months ago (2016-06-01 22:10:40 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/75008)
4 years, 6 months ago (2016-06-01 22:36:16 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008553007/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008553007/160001
4 years, 6 months ago (2016-06-01 22:53:27 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/239120)
4 years, 6 months ago (2016-06-02 01:59:36 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008553007/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008553007/160001
4 years, 6 months ago (2016-06-02 05:54:21 UTC) #35
commit-bot: I haz the power
Committed patchset #9 (id:160001)
4 years, 6 months ago (2016-06-02 06:26:50 UTC) #36
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/804ed8a1e0f236f22e19abef7013e9641a3ad49d Cr-Commit-Position: refs/heads/master@{#397311}
4 years, 6 months ago (2016-06-02 06:27:52 UTC) #38
Nico
changes to third_party code should update some README.chromium to mention in what ways we're forking ...
4 years, 6 months ago (2016-06-03 19:46:58 UTC) #40
huangs
4 years, 6 months ago (2016-06-06 04:47:58 UTC) #41
Message was sent while issue was closed.
PagedArray is purely a Google addition.  It's kinda weird that it lives in this
directory.

There will be more activities in the area though, specifically, I will:
- Make licensing comments more uniform in \courgette\third_party\bsdiff, and
remove ad hoc exceptions in tools/checklicenses/checklicenses.py
- Move BSDiff-specific code from courgette namespace to bsdiff namespace
- Replace QSufSort with divsufsort, which will require third_party code review.

thakis@: Would be interested in being CC'ed on these issues?

Powered by Google App Engine
This is Rietveld 408576698