[runtime] use std::vector in KeyAccumulator

[runtime] Use std::vector in KeyAccumulator

LOG=N
BUG=chromium:545503
Committed: https://crrev.com/c043a7eee13e7a41574706f79325dc3cc14d26c9
Cr-Commit-Position: refs/heads/master@{#31557}

[Camillo Bruni, 2015-10-21 15:45:39]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-21 15:45:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409073005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409073005/1

[commit-bot: I haz the power, 2015-10-21 15:51:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-21 15:51:56]

Dry run: Try jobs failed on following builders:
  v8_win_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/10898)

[Camillo Bruni, 2015-10-22 11:16:56]

cbruni@chromium.org changed reviewers:
+ ishell@chromium.org

[Camillo Bruni, 2015-10-22 11:16:57]

PTAL

[Camillo Bruni, 2015-10-22 11:17:35]

Description was changed from

==========
[runtime] use std::vector in KeyAccumulator

LOG=N
BUG=chromium:545503
==========

to

==========
[runtime] Use std::vector in KeyAccumulator

LOG=N
BUG=chromium:545503
==========

[Igor Sheludko, 2015-10-22 21:42:57]

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc#newcode7505
src/objects.cc:7505: for (uint32_t i = 0; i < elements_.size(); i++) {
I guess it should be size_t or you will have compilation issues on 64-bit.

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc#newcode7526
src/objects.cc:7526: for (uint32_t level = 0; level < levelLengths_.size();
level++) {
Ditto.

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc#newcode7577
src/objects.cc:7577: for (uint32_t i = 1; i < elements_.size(); i++) {
Ditto.

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc#newcode7685
src/objects.cc:7685: // hence we set INCLUDE_CURRENT_LEVEL
DCHECK_EQ(EXCLUDE_CURRENT_LEVEL, elementKeysAddCheckLimit_);
or maybe save the old value to return to.

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc#newcode7686
src/objects.cc:7686: elementKeysAddCheckLimit_ = INCLUDE_CURRENT_LEVEL;
WDYT about adding all the elements without checking for duplicates in the last
level, then sort the last level and erase consecutive duplicates instead of
doing linear search each time?
  std::sort(last_level.begin(), last_level.end());
  last_level.erase(std::unique(last_level.begin(), last_level.end()),
last_level.end());
  and adjust the lengths according to the number of elements deleted.

https://codereview.chromium.org/1409073005/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1409073005/diff/40001/src/objects.h#newcode10780
src/objects.h:10780: std::vector<std::vector<uint32_t>*> elements_;
Suggestion: use std::unique_ptr:
  std::vector<std::unique_ptr<std::vector<uint32_t>>> elements_;
In this case you can remove destructor.

https://codereview.chromium.org/1409073005/diff/40001/src/objects.h#newcode10792
src/objects.h:10792: ElementKeyAddCheckLimit elementKeysAddCheckLimit_ =
EXCLUDE_CURRENT_LEVEL;
Nice! What about initializing lengths fields this way?

[Camillo Bruni, 2015-10-23 08:56:35]

PTAL, adressing nits

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc#newcode7505
src/objects.cc:7505: for (uint32_t i = 0; i < elements_.size(); i++) {
On 2015/10/22 at 21:42:57, Igor Sheludko wrote:
> I guess it should be size_t or you will have compilation issues on 64-bit.

right

https://codereview.chromium.org/1409073005/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1409073005/diff/40001/src/objects.h#newcode10780
src/objects.h:10780: std::vector<std::vector<uint32_t>*> elements_;
On 2015/10/22 at 21:42:57, Igor Sheludko wrote:
> Suggestion: use std::unique_ptr:
>   std::vector<std::unique_ptr<std::vector<uint32_t>>> elements_;
> In this case you can remove destructor.

I think I prefer not using smart pointers since we do not leak any objects, if
that is fine with you?

https://codereview.chromium.org/1409073005/diff/40001/src/objects.h#newcode10792
src/objects.h:10792: ElementKeyAddCheckLimit elementKeysAddCheckLimit_ =
EXCLUDE_CURRENT_LEVEL;
On 2015/10/22 at 21:42:57, Igor Sheludko wrote:
> Nice! What about initializing lengths fields this way?

done

[Camillo Bruni, 2015-10-23 10:58:56]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-23 10:59:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409073005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409073005/80001

[Camillo Bruni, 2015-10-23 12:39:08]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-23 12:39:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409073005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409073005/100001

[commit-bot: I haz the power, 2015-10-23 12:50:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-23 12:50:08]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/11057)

[Camillo Bruni, 2015-10-26 09:53:48]

PTAL
fixed some issues with spares arrays

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1409073005/diff/40001/src/objects.cc#newcode7686
src/objects.cc:7686: elementKeysAddCheckLimit_ = INCLUDE_CURRENT_LEVEL;
On 2015/10/22 at 21:42:57, Igor Sheludko wrote:
> WDYT about adding all the elements without checking for duplicates in the last
level, then sort the last level and erase consecutive duplicates instead of
doing linear search each time?
>   std::sort(last_level.begin(), last_level.end());
>   last_level.erase(std::unique(last_level.begin(), last_level.end()),
last_level.end());
>   and adjust the lengths according to the number of elements deleted.

ah right, makes sense :)

[Igor Sheludko, 2015-10-26 10:22:04]

lgtm with suggestion:

https://codereview.chromium.org/1409073005/diff/140001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1409073005/diff/140001/src/objects.h#newcode1...
src/objects.h:10821: void SortRemoveDuplicates();
WDYT: SortCurrentElementsListRemoveDuplicates()?

[Camillo Bruni, 2015-10-26 10:46:07]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2015-10-26 10:46:08]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/1409073005/#ps160001
(title: "nits")

[commit-bot: I haz the power, 2015-10-26 10:46:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409073005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409073005/160001

[commit-bot: I haz the power, 2015-10-26 10:47:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-26 10:47:51]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)

[Camillo Bruni, 2015-10-26 11:27:51]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2015-10-26 11:27:51]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/1409073005/#ps180001
(title: "typo")

[commit-bot: I haz the power, 2015-10-26 11:28:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409073005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409073005/180001

[commit-bot: I haz the power, 2015-10-26 11:46:39]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-10-26 11:47:11]

Patchset 10 (id:??) landed as
https://crrev.com/c043a7eee13e7a41574706f79325dc3cc14d26c9
Cr-Commit-Position: refs/heads/master@{#31557}

[Camillo Bruni, 2015-10-26 13:56:01]

https://codereview.chromium.org/1409073005/diff/140001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1409073005/diff/140001/src/objects.h#newcode1...
src/objects.h:10821: void SortRemoveDuplicates();
On 2015/10/26 at 10:22:04, Igor Sheludko wrote:
> WDYT: SortCurrentElementsListRemoveDuplicates()?

indeed better :)