4.0 - #5
4.0#5
Conversation
Zephir buffer
|
Post-merge review of #5 (4.0: PHP arrays → C buffers) Thanks for this. The direction is right and the speedups are real. I built the PR head ( What's good
What needs follow-up (details inline)
I can turn any of these into PRs. Inline comments1 ·
|
| PHP | mismatches (of 800k) |
|---|---|
| 8.1.34 | 0 |
| 8.3.33 | 0 |
| 8.4.26 | 17,498 |
| 8.5.11 | 17,498 |
round(0.49999999999999994, 0): php=0 ext=1
round(1.4999999999999998, 0): php=1 ext=2
round(178.54999999999998, 1): php=178.5 ext=178.6
round(4503599627370495.5, 0): php=4503599627370496 ext=4503599627370495.5
The simplest fix that is correct on every version is to call the engine's implementation. It is PHPAPI on 8.1 through 8.5, and Zephir's kernel/math.c already uses it:
#include <ext/standard/php_math.h>
...
vc[i] = _php_math_round(va[i], places, PHP_ROUND_HALF_UP);The local port and its lookup tables can then go.
9 · tensor/matrix.zep · L1927–L1945
[Medium] repeat($m, 0) returns an empty matrix
The docblock and the C kernel agree that the result is m·(times_m+1) × n·(times_n+1), but the early return short-circuits every call with n < 1:
$a = Matrix::fromArray([[1.0], [2.0]]);
$a->repeat(0, 0); // 0×0; expected 2×1 (the same matrix)
$a->repeat(1, 0); // 0×0; expected 4×1
$a->repeat(1, -1); // 0×0, silently
$a->repeat(-1, 1); // \InvalidArgumentException (SPL) "Times must be non-negative." raised from CThis hits real code. Rubix ML's Hyperplane generator does ->asColumnMatrix()->repeat(0, $d - 1), so new Hyperplane([2.5]) gets a 0×0 matrix. The next ->multiply($coefficients) then throws DimensionalityMismatch: Matrix A expects 0 columns but Vector B has 1.
The bug is inherited: 3.x wrapped the row loop in if n > 0, and rubix/tensor's Matrix::repeat() has the same if ($n > 0). Since this PR rewrites the method anyway, I'd fix it here (and in the PHP library) instead of making it explicit:
if unlikely m < 0 || n < 0 {
throw new InvalidArgumentException("Repeat counts must be"
. " greater than or equal to 0.");
}
if unlikely this->m < 1 || this->n < 1 {
return new self(tensor_buffer_from_array([]), this->m * (m + 1), this->n * (n + 1));
}10 · optimizers/TensorReduceSumOptimizer.php · L66–L68 (the same pattern is in every optimizer; see also ext/include/arithmetic.c L26–L30)
[Medium] Errors raised in C: SPL exception classes, and the generated code keeps running after them
-
The kernels throw
\LengthException, SPL\InvalidArgumentExceptionand\OutOfBoundsException, while the Zephir layer and AGENTS.md useTensor\Exceptions\*.catch (TensorException $e)no longer catches a dimension mismatch that only the kernel detects:$a = Matrix::fromArray([[1.0, 2.0], [3.0, 4.0]]); $b = new Matrix(Vector::fromArray([1.0, 2.0, 3.0])->asTensorBuffer(), 2, 2); try { $a->multiplyMatrix($b); } catch (Tensor\Exceptions\TensorException $e) { // not reached: LengthException: Input buffers must be the same length. }
I'd suggest
tensor_exceptions_invalidargumentexception_ce/tensor_exceptions_dimensionalitymismatch_ce.tensor_buffer_repeat()already uses the former in one place. -
The optimizers emit a bare
tensor_xxx(&result, ...);with no exception check. After a kernel throws, the method keeps running withresultstill NULL. If the next statement calls a method onresult, the real error is buried under anError:// Vector::sum(): tensor_reduce_sum() throws, then result->get(0) runs on NULL Error: Call to a member function get() on null previous: LengthException: Matrix and row dimensions must agree.Having the optimizer template emit the same exception check that Zephir emits after its own calls (for example
if (UNEXPECTED(EG(exception))) { RETURN_MM(); }) would fix every call site at once.
11 · CHANGELOG.md · L3–L4
[Medium] The 4.0.0 changelog doesn't list the breaking changes
The only entry is the fromArray() speed-up, and on master (1003b1c) even that line is gone, leaving 4.0.0 empty. People upgrading the extension need to know at least:
Vector/ColumnVector/Matrixbuild()andquick()were removed; usefromArray($a, $validate)instead.- Constructors now take
(TensorBuffer $a)or(TensorBuffer $a, int $m, int $n). - New
Tensor\BufferandTensor\TensorBufferclasses, and newasTensorBuffer(),asRowBuffers(),asColumnBuffers(),argmin()andargmax()methods. mod*()now usesfmod(), so results are no longer truncated to integers. Divisors in (-1, 1) used to throwDivisionByZeroError, because they truncate to 0. Nowfmod()returns a value for them, and a zero divisor givesNAN. For example,[5.5, 7.9] mod 2is[1.5, 1.9](3.1.1:[1, 1]), and[5.5] mod 0.5is[0.0](3.1.1:DivisionByZeroError).rubix/tensor4.0 documents this as "Modulus results no longer rounded to nearest integer".- The
Specialinterface gainedargmin()/argmax(), so third-partyTensorimplementations must add them. - The serialization format changed (comment 4).
fromArray()is positional and discards keys.- Numeric-string offsets such as
$vector['0']now throw (3.1.1 returned the element). Matrix::asArray()on an m×0 matrix returns[](3.1.1: m empty rows), and reductions on 0-row matrices throw (3.1.1: an emptyColumnVector).min()/max()with NaN now depend on its position:[NAN, 1.0]->min()isNAN(3.1.1:1.0).- Errors raised by the kernels are SPL exceptions.
round()behaves differently on PHP ≥ 8.4, unless comment 8 is fixed.
Keeping quick() and build() for one major version as deprecated one-line aliases (fromArray($a, false) / fromArray($a)) would make the upgrade painless for anyone not already on Rubix ML master.
12 · tensor/matrix.zep · L1862–L1917 (see also tensor/vector.zep L2074–L2082)
[Medium, perf] Slower than 3.1.1 on paths that cross the PHP/C boundary
These are rough single runs (PHP 8.4, arm64, OpenBLAS single-threaded), in µs per call. The kernel paths are a huge win, but several common paths got slower:
| operation | 3.1.1 | 4.0 | |
|---|---|---|---|
A + A (2000×64) |
1500 | 28 | 53× faster |
transpose (2000×64) |
850 | 44 | 19× faster |
row sum (2000×64) |
238 | 20 | 12× faster |
variance (2000×64) |
2720 | 204 | 13× faster |
matmul (256×256) |
2807 | 963 | 2.9× faster |
$v[$i] loop, 100k |
5777 | 19801 | 3.4× slower |
$m[$i][$j] loop, 2000×64 |
7627 | 29657 | 3.9× slower |
augmentRight (2000×64) |
330 | 3994 | 12× slower |
foreach ($matrix as $row) |
509 | 1207 | 2.4× slower |
asVectors() |
544 | 1184 | 2.2× slower |
Matrix::fromArray vs quick (2000×64) |
33 | 882 | O(1) → O(n) |
asArray() (2000×64) |
0.1 | 589 | O(1) → O(n) |
Ideas:
augmentLeft/Rightbuildmslices, thenmconcats, then callfromBuffers(). A C kernel that interleaves the rows with twomemcpys per row would remove all of that. Rubix ML'sRidge::train()callsaugmentLeft()on the whole dataset.Vector::offsetGet()goes through a Zephir method, thenTensorBuffer::get()(another method call), then the kernel dimension read. An optimizer that reads the double straight fromtensor_tensorbuffer_doubles()would save two method calls per element.Matrix::offsetGet()has the same problem and allocates a Buffer, a TensorBuffer and a Vector per row.zeros(),ones(),fill(),identity()anddiagonal()build PHP arrays and then flatten them.new Buffer($n)is already zero-filled, andBuffer::fill()exists.Matrix::fromArray()still builds a flat PHP array before copying it into the buffer. Walking the nested array straight into the buffer in C would avoid creating an m·n zval array.- The
fromArraybenchmarks added here were removed on master in 1003b1c. I'd keep them and add benchmarks for element access, iteration andaugment*, so regressions like these show up.
13 · ext/include/buffer.h · L14–L19 (see also ext/kernel/buffer.h L81–L85, ext/include/signal_processing.c L37–L44)
[Low] "Returns FAILURE (and throws)", but nothing is thrown
zephir_buffer_create() and tensor_tensorbuffer_create() are documented to throw on failure, but for a negative length they just return FAILURE. Every kernel then does return;, with no exception and no return value, and Zephir passes NULL to the next constructor:
Vector::fromArray([])->convolve(Vector::fromArray([]));
// TypeError: Tensor\Vector::__construct(): Argument #1 ($a) must be of type Tensor\TensorBuffer, null givenHere nc = na + nb - 1 = -1 in tensor_convolve_1d. Either throw from tensor_tensorbuffer_create() on failure, or guard the callers. convolve() with an empty kernel should probably be an InvalidArgumentException; [1, 2, 3]->convolve([]) currently returns [0, 0].
14 · ext/include/linear_algebra.c · L97
[Low] BLAS gets called with lda = 0 for empty dimensions
Matrix::fromArray([[], []])->dot(Vector::fromArray([]));
// printed by OpenBLAS: ** On entry to DGEMV parameter number 6 had an illegal valueOpenBLAS's xerbla writes straight to the process's stdout, bypassing PHP's output layer and error handling. Returning early when ma == 0 || pc == 0 avoids it. The other BLAS/LAPACK calls are worth checking for zero-sized dimensions (or passing MAX(1, ld)) too.
15 · tensor/matrix.zep · L599–L606 (see also ext/include/reductions.c L244–L249)
[Low] Degenerate shapes behave differently from 3.1.1
Matrix::fromArray([[], [], []])->asArray(); // [] (3.1.1: [[], [], []])
Matrix::fromArray([])->sum(); // \InvalidArgumentException "Number of groups must be greater than 0." (3.1.1: empty ColumnVector)
// mean(), l2Norm(), covariance() and median() on 0-row matrices throw as well
Matrix::fromArray([[], []])->covariance(); // LengthException: Matrix and vector dimensions must agree.The n < 1 early returns drop m. For the reductions, tensor_reduce_apply() could return an empty buffer when groups == 0 instead of throwing. (transpose() of m×0 → 0×0 was already the case in 3.1.1.)
16 · ext/include/buffer.c · L140–L151 (see also ext/include/reductions.c L181–L215, L360–L366)
[Low] NaN handling depends on where the NaN sits
(da > db) - (da < db) returns 0 for any comparison with NaN, so it isn't a strict weak ordering. Once a NaN is present, qsort() results are arbitrary, and with them median(), quantile() and TensorBuffer::sort(). min, max, argmin and argmax keep the NaN only if it's the first element:
Vector::fromArray([NAN, 1.0, 3.0, 2.0])->median(); // 1.5
Vector::fromArray([NAN, 1.0])->min(); // NAN (3.1.1: 1.0)
Vector::fromArray([1.0, NAN])->min(); // 1.0A NaN-aware comparator (for example, NaN sorts last) and a documented rule for the extrema (propagate NaN, or ignore it) would make this deterministic. The two comparators are identical, so one shared helper would do. A consistent comparator is also worth having for its own sake: older glibc qsort() fallbacks had memory-safety bugs with non-transitive comparators on very large arrays.
17 · ext/include/arithmetic.c · L413 (see also ext/include/linear_algebra.c L41, L386–L394)
[Low] Dimension checks multiply zend_longs without overflow checks (UB), and sizes use unsigned int
Checks such as total != m * nHat, na != ma * pa || nbb != pa * nb and total != ma * na multiply values taken from the unvalidated shape (comment 5). A signed overflow there is undefined behaviour, and a wrapped product can make the check pass. In the decomposition kernels, the dimensions are cast to unsigned int and the allocation sizes are computed as ma * na * sizeof(double) in 32-bit arithmetic before widening. None of this is reachable with sane shapes, but these checks are the last line of defence for comment 5. tensor_buffer_slice_strided() already shows the fix (divide instead of multiply), and safe_emalloc(ma, (size_t) na * sizeof(double), 0) covers the allocation side.
18 · composer.json · L34 (see also .github/workflows/ci.yml)
[Low] Build reproducibility and CI coverage
"phalcon/zephir": "dev-development"with no lock file. The committedext/depends on an unreleased Zephir branch: the newkernel/buffer.c,kernel-classes, and thetensor_buffer_cealias workaround inconfig.json. Regenerating later will silently pick up whateverdevelopmentis by then. Pinning a commit (dev-development#<sha>) until a Zephir release shipsBufferwould makecomposer compilereproducible.- CI only triggers on
ext/**,tests/**and the workflow itself. A PR that changestensor/*.zep,optimizers/**orconfig.jsonwithout regeneratingext/therefore passes. A job that runszephir generatefollowed bygit diff --exit-code ext/would catch that. For what it's worth, I checked heuristically that the committed C matches the.zepat522dfef: all 178zephir_throw_exception_debugsites point at the matchingthrowlines. - PHPStan was removed from CI and composer, but
stubs/TensorBuffer.phpstill says it exists forscanFilesinphpstan.neon, which has been deleted. Either restore the analysis (the stub was written to make it work) or drop the stub. - An ASan job would be cheap: build with
-fsanitize=address, run withUSE_ZEND_ALLOC=0, and preload a tiny shim that stripsRTLD_DEEPBINDfrom PHP'sdlopen(). Together with edge-case tests (empty and tall matrices, NaN, huge repeat counts), it would have flagged all three High findings above.
19 · Nits (one comment, or spread across the files)
AGENTS.mdL79:extension=iconvis missing its-d, so PHP takes it as the script path and PHPUnit never runs. Also: "TEnsor" (L76); the "Full build" row (L35) still listsanalyze, butcomposer buildno longer runs it; "Bump the version in bothconfig.json" (L70) now names only one file.scripts/compare-serialization.phpL27: the hint points atext/modules/tensor.so, but the module istensor_ext.so.docs/TensorBuffer.md:get()andset()throw SPLRuntimeException/OutOfBoundsException(from the kernelBuffer), notTensor\Exceptions\RuntimeException.fromBuffers()is undocumented.concat()with an element that isn't aTensorBufferraisesError: Call to a member function asBuffer(), notInvalidArgumentException.
tensor/tensorbuffer.zepL130–L182:sliceStrided()hand-rolls an overflow-safe multiplication. The C side already re-checks with(length - 1) > last / stride, so the Zephir side could use the same one-liner:length - 1 > intdiv(limit, stride).ext/php_tensor.h,ext/tensor.candext/tensor.hare leftovers from the old module name and aren't inconfig.m4. Even so,php_tensor.hreceived the newZEPHIR_BUFFER_*defines, which makes it easy to edit the wrong file.
Swaps PHP array for C array for all tensors.