Use HT for recursion protection in JSON encode

[nikic]

The jsonSerialize() method might access recursion-protected objects/arrays, in which case other operations like var_dump() may break. This also fixes https://bugs.php.net/bug.php?id=81524.

@tstarling Thoughts?

[tstarling]

The instruction count increases by about 40% for a tight loop, from 350 to 493 instructions per element. GC_PROTECTED is fast and will be hard to beat, but 40% seems like a lot.

$a = [];
for ($i = 0; $i < 1000; $i++) {
	$a[] = [$i];
}
for ( $i = 0; $i < 100000; $i++ ) {
	json_encode($a);
}

[nikic]

Yeah, the overhead here is non-trivial. Don't really see how to improve on it though.

I think the two alternatives would be a) simply ignore any weird interactions by different recursion protections -- it rarely matters or b) switch to a two-level recursion protection: The first user can make use of the GC_PROTECTED flag, while a nested second user would fall back to an HT instead. This would ensure that there is both little performance impact for the average case, and that there is no interference between different recursers. This would be a more intrusive change that needs to cover other users of GC_PROTECTED as well though.

[tstarling]

Object handles could be used as an index into an array of bits -- you could realloc a global array as more handles appear. The relative memory overhead would only be 1/8/sizeof(zend_object) = 0.2%. Doesn't help with arrays, and there's that TODO in the code about removing zend_object.handle, but maybe it could solve the bug at least.

[bukka]

I think as there's already json specific constant in Zend code ( ZEND_PROP_PURPOSE_JSON ) it might just be easier to also have a special protection for JSON. Basically the patch attached in the bug from @tstarling with renaming GC_PROTECTED2 to something like GC_PROTECTED_JSON which would be used only by JSON ext.

[nikic]

@bukka That would require reserving an additional GC bit. I don't think that makes sense for a JSON-only use case.

In any case, closing this one as the overhead is too large.

[TysonAndre]

Leaving a note if anyone bases functionality on this in the future or restores this:

As I'd discovered in #7690 - using pointers as hash indexes directly leads to a lot of hash collisions and performance issues. Shifting by ZEND_MM_ALIGNED_OFFSET_LOG2 instead helps noticeably (to work with both malloc and emalloc)

(e.g. if 44-byte zend_array instances (on 64-bit platforms) are aligned to 16 bytes in practice with emalloc on a platform (low bit of a pointer is the byte address), then they'll all collide on the same 1 in 16 hash buckets)

[TysonAndre]

So, the original code was increasing the reference count of the properties table, and decreasing the reference count of the properties table, to prevent the property table from getting freed during iteration.

Now that we're referencing the object, your PR is calling GC_ADDREF and GC_DELREF to prevent the object from getting freed during iteration.

If this PR were to be reopened or a if someone were to base code on this in the future, would it need to check if GC_DELREF returns 0 and free the array/object in question, to avoid leaks if jsonSerialize removed the last reference to a value as a side effect? i.e. call rc_dtor_func if unexpectedly 0

ZEND_API void ZEND_FASTCALL rc_dtor_func(zend_refcounted *p)

[TysonAndre]

• With the existing HashTable, this can shift pointers by ZEND_MM_ALIGNED_OFFSET_LOG2 and see some performance improvement. C apis for efficiently adding/checking if a pointer is in a HashTable, reducing hash collisions? #9813

---

A brand new type for unordered sets of non-null (non-0) pointers (only supporting adding, removal, and membership checks) might have even better performance (only for internal use within json.h, not an exported api).

https://github.com/igbinary/igbinary/blob/master/src/php7/hash_si_ptr.c along the lines of what is used there

• For a hash table of pointers/zend_long that only needs addition, membership check, and removal, there's no need for allocating memory for buckets as 16-byte zvals

• There doesn't even need to be an associated 16-byte zval in this case

• If values are always added in order and removed in the opposite order, I think there's no need to mark buckets as wasted. They can be reset to 0/null instead

  That assumption is sadly almost definitely wrong because of Fibers, though, since JsonSerialize::serialize can switch to a different fiber, that fiber can start a call to json_encode, which can switch back to the original call

[TysonAndre]

Separately from the previous comment, there's the question of whether the json recursion protection should be per-request (in request globals) rather than per call to json_encoder (in request globals, in RINIT/RSHUTDOWN) instead.

My preference is for the former

E.g. a JsonSerializable::jsonSerialize implementation calling json_encode($this) would trigger infinite recursion with this PR (but not before this PR) if each call to json_encode had a distinct recursive hash table instance - since object property tables don't really change to different pointers in practice if it's necessary for json_encode to work.

[bukka]

Just for the record this was addressed by 53aa53f