[ignition] Prefill the constant array with holes to avoid needing to write padding holes

[ignition] Prefill the constant array with holes to avoid needing to write padding holes

BUG=

Review-Url: https://codereview.chromium.org/2586513002
Cr-Commit-Position: refs/heads/master@{#42139}
Committed: https://chromium.googlesource.com/v8/v8/+/7256d90a35faf5d8cf52773fca06e6ad7a1e1ba4

[Leszek Swirski, 2016-12-16 16:50:53]

leszeks@chromium.org changed reviewers:
+ leszeks@chromium.org

[Leszek Swirski, 2016-12-16 16:50:54]

lgtm other than one nit

https://codereview.chromium.org/2586513002/diff/1/src/interpreter/constant-ar...
File src/interpreter/constant-array-builder.cc (left):

https://codereview.chromium.org/2586513002/diff/1/src/interpreter/constant-ar...
src/interpreter/constant-array-builder.cc:154: DCHECK_EQ(array_index,
fixed_array->length());
I would keep this check, as a DCHECK_GE

[Toon Verwaest, 2016-12-16 16:56:22]

Addressed comment, thanks

[Toon Verwaest, 2017-01-09 10:35:04]

Fixed overflow, ptal again

[Leszek Swirski, 2017-01-09 10:47:08]

lgtm

[rmcilroy, 2017-01-09 11:59:04]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2017-01-09 11:59:05]

LGTM

[Toon Verwaest, 2017-01-09 12:00:23]

The CQ bit was checked by verwaest@chromium.org

[commit-bot: I haz the power, 2017-01-09 12:00:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 12:24:54]

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

[commit-bot: I haz the power, 2017-01-09 12:24:55]

Failed to apply patch for src/interpreter/constant-array-builder.cc:
While running git apply --index -p1;
  error: patch failed: src/interpreter/constant-array-builder.cc:126
  error: src/interpreter/constant-array-builder.cc: patch does not apply

Patch:       src/interpreter/constant-array-builder.cc
Index: src/interpreter/constant-array-builder.cc
diff --git a/src/interpreter/constant-array-builder.cc
b/src/interpreter/constant-array-builder.cc
index
d2b799562383dd87c5210509bff00061436d6573..fb8e9bd7deeaa81c5348b9a4ec410c86a4137c7e
100644
--- a/src/interpreter/constant-array-builder.cc
+++ b/src/interpreter/constant-array-builder.cc
@@ -126,13 +126,10 @@ Handle<FixedArray>
ConstantArrayBuilder::ToFixedArray(Isolate* isolate) {
                          handle(reserved_smi.first, isolate));
   }
 
-  Handle<FixedArray> fixed_array = isolate->factory()->NewFixedArray(
+  Handle<FixedArray> fixed_array = isolate->factory()->NewFixedArrayWithHoles(
       static_cast<int>(size()), PretenureFlag::TENURED);
   int array_index = 0;
   for (const ConstantArraySlice* slice : idx_slice_) {
-    if (array_index == fixed_array->length()) {
-      break;
-    }
     DCHECK(array_index == 0 ||
            base::bits::IsPowerOfTwo32(static_cast<uint32_t>(array_index)));
     // Different slices might contain the same element due to reservations, but
@@ -143,15 +140,14 @@ Handle<FixedArray>
ConstantArrayBuilder::ToFixedArray(Isolate* isolate) {
     for (size_t i = 0; i < slice->size(); ++i) {
       fixed_array->set(array_index++, *slice->At(slice->start_index() + i));
     }
-    // Insert holes where reservations led to unused slots.
-    size_t padding =
-        std::min(static_cast<size_t>(fixed_array->length() - array_index),
-                 slice->capacity() - slice->size());
-    for (size_t i = 0; i < padding; i++) {
-      fixed_array->set(array_index++, *the_hole_value());
+    // Leave holes where reservations led to unused slots.
+    size_t padding = slice->capacity() - slice->size();
+    if (static_cast<size_t>(fixed_array->length() - array_index) <= padding) {
+      break;
     }
+    array_index += padding;
   }
-  DCHECK_EQ(array_index, fixed_array->length());
+  DCHECK_GE(array_index, fixed_array->length());
   return fixed_array;
 }

[Toon Verwaest, 2017-01-09 12:31:06]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2017-01-09 12:31:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, leszeks@chromium.org
Link to the patchset: https://codereview.chromium.org/2586513002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-09 12:31:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 13:42:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483965066126220,
"parent_rev": "09167bf6cd4aaa321803eb8f4261295840e89730", "commit_rev":
"7256d90a35faf5d8cf52773fca06e6ad7a1e1ba4"}

[commit-bot: I haz the power, 2017-01-09 13:42:05]

Description was changed from

==========
[ignition] Prefill the constant array with holes to avoid needing to write
padding holes

BUG=
==========

to

==========
[ignition] Prefill the constant array with holes to avoid needing to write
padding holes

BUG=

Review-Url: https://codereview.chromium.org/2586513002
Cr-Commit-Position: refs/heads/master@{#42139}
Committed:
https://chromium.googlesource.com/v8/v8/+/7256d90a35faf5d8cf52773fca06e6ad7a1...
==========

[commit-bot: I haz the power, 2017-01-09 13:42:06]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/7256d90a35faf5d8cf52773fca06e6ad7a1...