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

Improved loop for getAllQueries #2114

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Improved loop for getAllQueries #2114

merged 1 commit into from
Feb 11, 2022

Conversation

rdwebdesign
Copy link
Member

@rdwebdesign rdwebdesign commented Feb 9, 2022

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

This PR reduces the memory needed by api_db.php?getAllQueries to process every requested record.

How does this PR accomplish the above?:

The current code used to generate and return the request adds all records into a single array. This array grows for each record (and there are many many records).
Only at the end (and if memory holds up), it transforms the entire array into JSON and sends all at once as a string.

The new code outputs each record as a string immediately.
This way, there is no array and the memory for each iteration is always the same.
The web server is responsible for sending the "text stream", nothing else needs to change.

What documentation changes (if any) are needed to support this PR?:

None.

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@DL6ER
Copy link
Member

DL6ER commented Feb 9, 2022

Does this output in chunks instead of one big output suggest adding header("Transfer-Encoding: chunked"); ?

@rdwebdesign
Copy link
Member Author

rdwebdesign commented Feb 9, 2022

The problem here is not exactly how the page outputs the result.

The problem is how much memory the page needs during the process.

The old code holds ALL data in memory during the entire while loop.
Each loop iteration adds a new item to the array (the array grows).
At the end of the loop, there is a huge array in memory.
Only after the loop the page processes the array and outputs the result.
There will be no memory gain using "chunked" output, because the memory was already full.

The new code outputs one record at a time.
Each loop iteration holds only the current record in memory. The memory use is very lower.

@DL6ER
Copy link
Member

DL6ER commented Feb 9, 2022

The new code outputs one record at a time.

Yes, hence my question because this is now chunked.

@rdwebdesign
Copy link
Member Author

For PHP this won't change anything.
This header forces the web server to send a "chunked" response.

As far as I know, Lighttpd automatically uses server.stream-response-body = 1 if needed.

@rdwebdesign rdwebdesign requested a review from yubiuser February 10, 2022 18:36
@PromoFaux PromoFaux self-requested a review February 11, 2022 22:23
@PromoFaux PromoFaux merged commit eaf2fe1 into devel Feb 11, 2022
@PromoFaux PromoFaux deleted the apidb_improvement branch February 11, 2022 22:23
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-14-web-v5-11-and-core-v5-9-released/53529/1

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/all-queries-are-not-working/54711/7

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/dreaded-php-memory-error-persists/55790/2

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.

4 participants