Skip to content

Performance improvements#1621

Draft
NikoGrano wants to merge 13 commits intoschmittjoh:masterfrom
NikoGrano:performance
Draft

Performance improvements#1621
NikoGrano wants to merge 13 commits intoschmittjoh:masterfrom
NikoGrano:performance

Conversation

@NikoGrano
Copy link
Copy Markdown
Contributor

Improve library performance. See commits for exact details.

Q A
Bug fix? yes/no
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Types like "string", "DateTime<'Y-m-d'>", "array<MyClass>" are re-parsed on every serialize/deserialize call. Cache eliminates redudant lexer/parser work. Wired as default in SerializerBuilder; users calling setTypeParser() still override entirely.
Avoid creating new ReflectionClass and running array_search() on every call. Use a static flipped constants map for O(1) lookup.
Replace O(n) linear scan with O(1) hash lookup using a static flipped array.
Comment thread src/Type/Parser.php
self::$constantNames = array_flip((new \ReflectionClass(Lexer::class))->getConstants());
}

return array_search($value, $oClass->getConstants());
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array_search defaulted to false if not found and would have caused crash as function has expected output of string.

After this change error might change, maybe add ?? false, but again this will just cause same type related error in php...

In my opinion this is such edge case it should not happen as it would mean this library being broken internally and would just add extra check which should not be ever needed. Any opinion on this could be helpful, or just leave it "as is".

Avoid creating new DateTimeZone objects on every deserialization call for the same timezone string. Instance-level caching.
Isolates type parsing performance: 9 realistic type strings, 1000 revs x 5 iterations, comparing raw Parser vs CachingParser.
uniqid() is just slow and not even cryptographically secure. The prefix only needs to be unique within a single XPath registerXPathNamespace() call, not globally. Replaced with simpler int counter.
…zationVisitor

Added instance-leven caches for resolveNamespacePrefix() avoiding repeated SHA1 calculation and isElementNameValid() avoiding repeated preg_match for the same keys.
Cache the handler result from the polymorphic type check and reuse it if the type hasn't changed after the pre_serialize event dispatch, eliminating a redundant hash lookup per object node.
…trategy

When nestedGroups is enabled, shouldSkipProperty() used in_array() O(n) to check group membership. Build a hashmap from getGroupsFor() result and use isset() O(1) instead.
Use three fast paths based on stack delta analysis to avoid full scans on every shouldSkipProperty()/shouldSkipClass() call. Same stack count retunrs cached result O(1). Stack shrunk and was not too deep O(1). Stack grew, no maxDepth on stack, not too deep, check only deltas O(delta). hasMaxDepthOnStack flag tracks if any maxDepth attributes exist on the stack. When false, growth never triggers full scan. When true or cazched result is too deep, the logic falls back to original full scan.
@NikoGrano

This comment was marked as outdated.

In PHP 7.x/8.x arrays outperform SplStack with noticable difference.
@NikoGrano
Copy link
Copy Markdown
Contributor Author

After these changes I can see some improvements (avg from 5 runs):

Benchmark Before After Δ Improvement
JsonSerializationBench 218.4ms 201.8ms −16.6ms 7.6%
JsonMaxDepthSerializationBench 273.7ms 249.4ms −24.3ms 8.9%
XmlSerializationBench 405.1ms 361.9ms −43.2ms 10.7%

Memory usage unchanged across all benchmarks (~39.2 MB for JSON, ~11.7 MB for XML).

@scyzoryck What do you think, is this something to finalize and prep for merge?

I might still go trough and try minimize changes, but at this stage this is what we would get if I decide to continue this PR.

@scyzoryck
Copy link
Copy Markdown
Collaborator

Hey!
I checked the results on the Github actions - I can see the difference on the XmlSerializationBench, but no difference in Json serialization :( I will check it on my local this week.

@NikoGrano
Copy link
Copy Markdown
Contributor Author

👍 Just a side note, I found this to be varying between envs. On my workstation I get clean results, but again with older laptop the difference is smaller. Especially PHP 7 vs PHP 8.

@scyzoryck
Copy link
Copy Markdown
Collaborator

@NikoGrano can you rebase to rerun tests with the PHP8.5?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants