Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce allocations #115

Merged
merged 2 commits into from
Jun 24, 2021
Merged

Reduce allocations #115

merged 2 commits into from
Jun 24, 2021

Conversation

mk6i
Copy link
Collaborator

@mk6i mk6i commented Jun 6, 2021

@crocodele In this branch AST nodes are retrieved from a stack-allocated array instead of dynamic allocation. When I run the benchmarks on the branch, the JsonPathPhpExtensionBenchmark beats NativePhpBenchmark on benchMediumDataset. Do you get the same results?

@mk6i mk6i closed this Jun 6, 2021
@mk6i mk6i reopened this Jun 6, 2021
@mk6i
Copy link
Collaborator Author

mk6i commented Jun 6, 2021

Would having opcache enabled close the performance gap between the implementations, thus rendering the extension moot? IIRC we would need to run this with PHP-FPM to test this out. (It's been like 5 years since I last developed with PHP, so I might be wrong.)

@crocodele
Copy link
Collaborator

When I run the benchmarks on the branch, the JsonPathPhpExtensionBenchmark beats NativePhpBenchmark on benchMediumDataset. Do you get the same results?

Hmm, no, I get pretty much the same numbers on refactor/reduce_alloc as on main. 🤔

The NativePhpBenchmark performance seems to depend on the system though to some extent. On my main machine I get values around 240 microseconds for benchMediumDataset, on another machine around 360 microseconds, which is slightly less than JsonPathPhpExtensionBenchmark on both machines.

Would having opcache enabled close the performance gap between the implementations, thus rendering the extension moot?

That's a good point. I ran the benchmarks again with opcache.enable_cli = 1. In the medium benchmark it cut 45% and 15% off the execution time for the two tested PHP libraries. The extension is still a lot faster though. No improvement from OPCache in the tiny or huge benchmarks.

@mk6i
Copy link
Collaborator Author

mk6i commented Jun 6, 2021

The NativePhpBenchmark performance seems to depend on the system though to some extent. On my main machine I get values around 240 microseconds for benchMediumDataset, on another machine around 360 microseconds, which is slightly less than JsonPathPhpExtensionBenchmark on both machines.

Interesting, did you try with both PHP 7.4 and 8?

@mk6i mk6i force-pushed the refactor/reduce_alloc branch 4 times, most recently from 4e4b2e7 to dbce73b Compare June 22, 2021 00:28
This commit aims to reduce the number of times emalloc() is called.
Instead of allocating arrays and strings each time every time a
literal or node list is compared in the interpreter, create literals and
node list once at parse time.

In addition, use a stack-allocated memory pool for AST nodes. This has
the benefit of eliminating calls to emalloc(), allocating contiguous
blocks of memory, and reducing the risk of future memory leaks.

The tradeoff we're making here is that there is a fixed number of AST
nodes (64), so there's a limit imposed on the size of the JSONPath
query. We'll see in the future if this poses a problem, in which case
we can implement Viktor's dynamic memory pool solution. (6cf186b)
@mk6i mk6i changed the title [WIP] Reduce allocations Reduce allocations Jun 22, 2021
@mk6i
Copy link
Collaborator Author

mk6i commented Jun 22, 2021

@crocodele I reverted the buggy dynamic memory pool. See commit message in 7a313cd for a summary of the changes.

@mk6i mk6i requested a review from crocodele June 22, 2021 02:04
src/jsonpath/parser.c Show resolved Hide resolved
This change reduces the size of the stack-allocated memory pool to
< 2Kib and removes the limit imposed on the number of elements in
the index filter.

With a bit of more work, we could support integer array operands.
(See tests/022.phpt)
@crocodele crocodele merged commit 8e87831 into main Jun 24, 2021
@crocodele crocodele deleted the refactor/reduce_alloc branch June 24, 2021 13:01
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.

None yet

2 participants