Fix simdjson_key_count for very large objects/arrays

TysonAndre · TysonAndre · commit ba5e18e5aa64 · 2022-10-01T19:23:06.000-04:00
If the value of size() is 0xFFFFFF, then iterate over the object/array instead to count the keys, because that's the largest value the C simdjson library can represent. Closes #74 CI improvements: - Install json module in CI for test cases to json_encode() - Run `make -j$(nproc)` in parallel to slightly speed up CI - Add json for tests in PHP < 8.0. (It's always enabled starting in PHP 8.0) - Enable json in CI images for tests with json(test using json_encode) Test case cleanup: Remove empty skipif sections and blank lines from tests.
diff --git a/.dockerignore b/.dockerignore
@@ -25,6 +25,7 @@ docker_*
 **/missing
 **/mkinstalldirs
 **/run-tests.php
+**/scripts
 **/Makefile
 **/Makefile.*
 **/!Makefile.frag
diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml
@@ -13,7 +13,7 @@ jobs:
         run: "docker build -t simdjsontest -f docker_php8.2-alpine ."
 
       - name: "Show"
-        run: "docker run --rm simdjsontest php --ri simdjson"
+        run: "docker run --rm --env SIMDJSON_HIGH_MEMORY_TESTS=1 simdjsontest php --ri simdjson"
 
   PHP81-alpine:
     name: PHP 8.1 Alpine 3.15
@@ -26,7 +26,7 @@ jobs:
         run: "docker build -t simdjsontest -f docker_php8.1-alpine ."
 
       - name: "Show"
-        run: "docker run --rm simdjsontest php --ri simdjson"
+        run: "docker run --rm --env SIMDJSON_HIGH_MEMORY_TESTS=1 simdjsontest php --ri simdjson"
 
   PHP80-alpine:
     name: PHP 8.0 Alpine 3.12
@@ -39,7 +39,7 @@ jobs:
         run: "docker build -t simdjsontest -f docker_php8.0-alpine ."
 
       - name: "Show"
-        run: "docker run --rm simdjsontest php --ri simdjson"
+        run: "docker run --rm --env SIMDJSON_HIGH_MEMORY_TESTS=1 simdjsontest php --ri simdjson"
 
   PHP74-alpine:
     name: PHP 7.4 Alpine 3.12
@@ -52,7 +52,7 @@ jobs:
         run: "docker build -t simdjsontest -f docker_php7.4-alpine ."
 
       - name: "Show"
-        run: "docker run --rm simdjsontest php --ri simdjson"
+        run: "docker run --rm --env SIMDJSON_HIGH_MEMORY_TESTS=1 simdjsontest php --ri simdjson"
 
   PHP73-alpine:
     name: PHP 7.3 Alpine 3.12
@@ -65,7 +65,7 @@ jobs:
         run: "docker build -t simdjsontest -f docker_php7.3-alpine ."
 
       - name: "Show"
-        run: "docker run --rm simdjsontest php --ri simdjson"
+        run: "docker run --rm --env SIMDJSON_HIGH_MEMORY_TESTS=1 simdjsontest php --ri simdjson"
 
   PHP72-alpine:
     name: PHP 7.2 Alpine 3.12
@@ -78,7 +78,7 @@ jobs:
         run: "docker build -t simdjsontest -f docker_php7.2-alpine ."
 
       - name: "Show"
-        run: "docker run --rm simdjsontest php --ri simdjson"
+        run: "docker run --rm --env SIMDJSON_HIGH_MEMORY_TESTS=1 simdjsontest php --ri simdjson"
 
   PHP71-alpine:
     name: PHP 7.1 Alpine 3.10
@@ -91,7 +91,7 @@ jobs:
         run: "docker build -t simdjsontest -f docker_php7.1-alpine ."
 
       - name: "Show"
-        run: "docker run --rm simdjsontest php --ri simdjson"
+        run: "docker run --rm --env SIMDJSON_HIGH_MEMORY_TESTS=1 simdjsontest php --ri simdjson"
 
   Ubuntu:
     strategy:
@@ -117,12 +117,14 @@ jobs:
         uses: shivammathur/setup-php@v2
         with:
           php-version: "${{ matrix.php-version }}"
+          extensions: json
           # for correct php-config extension dir, see https://github.com/shivammathur/setup-php/issues/147
           tools: pecl, phpize, php-config
       - name: "Build extension"
         run: |
           export NO_INTERACTION=true
           export REPORT_EXIT_STATUS=1
+          export SIMDJSON_HIGH_MEMORY_TESTS=1
           php-config --extension-dir
           phpize
           ./configure
diff --git a/docker_php7.1-alpine b/docker_php7.1-alpine
@@ -1,7 +1,8 @@
-# DO NOT MODIFY THIS AUTOGENERATED FILE
-# Change it in m4 folder
 FROM php:7.1.33-alpine3.10
 
+# json_encode is used in some phpt test cases
+RUN docker-php-ext-install -j$(nproc) json
+
 COPY . /tmp/simdjson
 
 # persistent / runtime deps
@@ -22,7 +23,7 @@ RUN set -xe \
     && cd /tmp/simdjson \
         && phpize  \
         && ./configure  \
-        && make  \
+        && make -j$(nproc)  \
         && make install \
         && make test \
         && rm -rf /tmp/simdjson \
diff --git a/docker_php7.2-alpine b/docker_php7.2-alpine
@@ -1,7 +1,8 @@
-# DO NOT MODIFY THIS AUTOGENERATED FILE
-# Change it in m4 folder
 FROM php:7.2-cli-alpine3.12
 
+# json_encode is used in some phpt test cases
+RUN docker-php-ext-install -j$(nproc) json
+
 COPY . /tmp/simdjson
 
 # persistent / runtime deps
diff --git a/docker_php7.3-alpine b/docker_php7.3-alpine
@@ -2,6 +2,9 @@
 # Change it in m4 folder
 FROM php:7.3-cli-alpine3.12
 
+# json_encode is used in some phpt test cases
+RUN docker-php-ext-install -j$(nproc) json
+
 COPY . /tmp/simdjson
 
 # persistent / runtime deps
@@ -22,7 +25,7 @@ RUN set -xe \
     && cd /tmp/simdjson \
         && phpize  \
         && ./configure  \
-        && make  \
+        && make -j$(nproc)  \
         && make install \
         && make test \
         && rm -rf /tmp/simdjson \
diff --git a/docker_php7.4-alpine b/docker_php7.4-alpine
@@ -2,6 +2,9 @@
 # Change it in m4 folder
 FROM php:7.4-cli-alpine3.12
 
+# json_encode is used in some phpt test cases
+RUN docker-php-ext-install -j$(nproc) json
+
 COPY . /tmp/simdjson
 
 # persistent / runtime deps
@@ -22,7 +25,7 @@ RUN set -xe \
     && cd /tmp/simdjson \
         && phpize  \
         && ./configure  \
-        && make  \
+        && make -j$(nproc)  \
         && make install \
         && make test \
         && rm -rf /tmp/simdjson \
diff --git a/docker_php8.0-alpine b/docker_php8.0-alpine
@@ -22,7 +22,7 @@ RUN set -xe \
     && cd /tmp/simdjson \
         && phpize  \
         && ./configure  \
-        && make  \
+        && make -j$(nproc)  \
         && make install \
         && make test \
         && rm -rf /tmp/simdjson \
diff --git a/docker_php8.1-alpine b/docker_php8.1-alpine
@@ -22,7 +22,7 @@ RUN set -xe \
     && cd /tmp/simdjson \
         && phpize  \
         && ./configure  \
-        && make  \
+        && make -j$(nproc)  \
         && make install \
         && make test \
         && rm -rf /tmp/simdjson \
diff --git a/docker_php8.2-alpine b/docker_php8.2-alpine
@@ -20,7 +20,7 @@ RUN set -xe \
     && cd /tmp/simdjson \
         && phpize  \
         && ./configure  \
-        && make  \
+        && make -j$(nproc)  \
         && make install \
         && make test \
         && rm -rf /tmp/simdjson \
diff --git a/package.xml b/package.xml
@@ -68,6 +68,7 @@
     <file name="key_count.phpt" role="test"/>
     <file name="key_count_args.phpt" role="test"/>
     <file name="key_count_exception.phpt" role="test"/>
+    <file name="key_count_large.phpt" role="test"/>
     <file name="key_exists.phpt" role="test"/>
     <file name="key_exists_args.phpt" role="test"/>
     <file name="key_value_args.phpt" role="test"/>
diff --git a/src/bindings.cpp b/src/bindings.cpp
@@ -327,17 +327,40 @@ void cplus_simdjson_key_count(simdjson::dom::parser& parser, const char *json, s
         return;
     }
 
-    zval v;
+    zend_long key_count;
     switch (element.type()) {
         //ASCII sort
-        case simdjson::dom::element_type::ARRAY : ZVAL_LONG(&v, uint64_t(element.get_array().value_unsafe().size()));
+        case simdjson::dom::element_type::ARRAY : {
+            auto json_array = element.get_array().value_unsafe();
+            key_count = zend_long(json_array.size());
+            if (UNEXPECTED(key_count == 0xFFFFFF)) {
+                /* The C simdjson library represents array sizes larger than 0xFFFFFF as 0xFFFFFF. */
+                key_count = 0;
+                for (auto it: json_array)  {
+                    key_count++;
+                }
+                ZEND_ASSERT(key_count >= 0xFFFFFF);
+            }
             break;
-        case simdjson::dom::element_type::OBJECT : ZVAL_LONG(&v, uint64_t(element.get_object().value_unsafe().size()));
+        }
+        case simdjson::dom::element_type::OBJECT : {
+            auto json_object = element.get_object().value_unsafe();
+            key_count = zend_long(json_object.size());
+            if (UNEXPECTED(key_count == 0xFFFFFF)) {
+                /* The C simdjson library represents object sizes larger than 0xFFFFFF as 0xFFFFFF. */
+                key_count = 0;
+                for (auto it: json_object) {
+                    key_count++;
+                }
+                ZEND_ASSERT(key_count >= 0xFFFFFF);
+            }
             break;
-        default: ZVAL_LONG(&v, 0);
+        }
+        default:
+            key_count = 0;
             break;
     }
-    *return_value = v;
+    ZVAL_LONG(return_value, key_count);
 }
 
 /* }}} */
diff --git a/tests/decode_args.phpt b/tests/decode_args.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_decode args test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $reflection = new \ReflectionFunction('\simdjson_decode');
diff --git a/tests/decode_exception.phpt b/tests/decode_exception.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_decode throws exception if json is invalid
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $json = "Invalid JSON string";
diff --git a/tests/decode_result.phpt b/tests/decode_result.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_decode test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $json = file_get_contents(__DIR__ . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'result.json');
diff --git a/tests/is_valid.phpt b/tests/is_valid.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_is_valid test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $json = file_get_contents(__DIR__ . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'result.json');
diff --git a/tests/is_valid_args.phpt b/tests/is_valid_args.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_is_valid args test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $reflection = new \ReflectionFunction('\simdjson_is_valid');
diff --git a/tests/key_count.phpt b/tests/key_count.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_key_count test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $json = file_get_contents(__DIR__ . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'result.json');
diff --git a/tests/key_count_args.phpt b/tests/key_count_args.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_key_count args test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $reflection = new \ReflectionFunction('\simdjson_key_count');
diff --git a/tests/key_count_exception.phpt b/tests/key_count_exception.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_key_count throws exception if key was not found test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $json = file_get_contents(__DIR__ . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'result.json');
diff --git a/tests/key_count_large.phpt b/tests/key_count_large.phpt
@@ -0,0 +1,43 @@
+--TEST--
+simdjson_key_count large value test
+--SKIPIF--
+<?php
+if (!function_exists('json_encode')) { echo "skip json_encode not available\n"; }
+if (PHP_INT_SIZE < 8) { echo "skip maybe not enough continuous memory for buffers to decode 200MB of json\n"; }
+if (!($_ENV['SIMDJSON_HIGH_MEMORY_TESTS'] ?? null)) { echo "skip requires SIMDJSONHIGH_MEMORY_TESTS=1\n"; }
+?>
+--INI--
+memory_limit=2G
+--FILE--
+<?php
+function my_array_repeat($value, int $n): array {
+    // This is too large for array_pad chunk size.
+    // Here, we choose a sma
+    $result = [];
+    for ($i = 0; $i < $n; $i++) {
+        $result[] = $value;
+    }
+    return $result;
+}
+function dump_count(array $x, int $flags = 0) {
+    // To reduce the amount of memory needed by this test:
+    // 1. Use the smallest possible value for the repeated array
+    // 2. Free the last reference to the array before using simdjson
+    $str = json_encode(['x' => $x], $flags);
+    unset($x);
+    var_dump(simdjson_key_count($str, "x"));
+}
+dump_count([]);
+dump_count([1 => '2']);
+dump_count(my_array_repeat(1, 0x1000000), JSON_FORCE_OBJECT);
+dump_count(my_array_repeat(1, 0x1000000));
+dump_count(my_array_repeat(1, 0x0FFFFFF), JSON_FORCE_OBJECT);
+dump_count(my_array_repeat(1, 0x0FFFFFF));
+?>
+--EXPECTF--
+int(0)
+int(1)
+int(16777216)
+int(16777216)
+int(16777215)
+int(16777215)
diff --git a/tests/key_exists.phpt b/tests/key_exists.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_key_exists test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $json = file_get_contents(__DIR__ . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'result.json');
diff --git a/tests/key_exists_args.phpt b/tests/key_exists_args.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_key_exists args test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $reflection = new \ReflectionFunction('\simdjson_key_exists');
diff --git a/tests/key_value_args.phpt b/tests/key_value_args.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_key_value args test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $reflection = new \ReflectionFunction('\simdjson_key_value');
diff --git a/tests/key_value_deep_string.phpt b/tests/key_value_deep_string.phpt
@@ -1,9 +1,5 @@
 --TEST--
 simdjson_key_value with deep string test
-
---SKIPIF--
-<?php ?>
-
 --FILE--
 <?php
 $json = file_get_contents(__DIR__ . DIRECTORY_SEPARATOR . '_files' . DIRECTORY_SEPARATOR . 'result.json');
diff --git a/tests/key_value_exception.phpt b/tests/key_value_exception.phpt
diff --git a/tests/key_value_int.phpt b/tests/key_value_int.phpt
diff --git a/tests/key_value_result.phpt b/tests/key_value_result.phpt
