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

Improve loop for getAllQueries from FTL's memory #2163

Merged
merged 5 commits into from
Apr 13, 2022
Merged

Improve loop for getAllQueries from FTL's memory #2163

merged 5 commits into from
Apr 13, 2022

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Apr 5, 2022

  • 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.php?getAllQueries to process every requested record.
It's basically a copy of #2114 but for the current in-memory data served by FTL.

Usually we have seen the out-of-memory error when requesting large amounts of data from the long-term database, but currently there is an issue on discourse where the in-memory data is also too huge to process getAllQueries

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

How does this PR accomplish the above?:
The old 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.

@yubiuser yubiuser requested a review from a team April 5, 2022 09:10
@yubiuser
Copy link
Member Author

yubiuser commented Apr 5, 2022

Has been confirmed working in the linked discourse topic.

@yubiuser yubiuser changed the title Improve loop for ?getAllQueries from FTL's memory Improve loop for getAllQueries from FTL's memory Apr 8, 2022
Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

There are merge conflicts.

I think it's worth noting that this removed a feature of the API that allows combining multiple requests in the same API call. I don't really know if it is every used for this endpoint, but we use it elsewhere. Example: Requests like api.php?summary&getAllQueries change behavior in this PR (summary is now ignored)

Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser
Copy link
Member Author

Merge conflicts resolved.

DL6ER made a good point. I searched for api.php? within the source and could not find any occurrence with getAllQueries and something else. But user scripts might use it. Not sure how to proceed. Is this a show stopper?
_

My last commit (moving the exit) made me think if the exit in this PR is also at the wrong spot? @rdwebdesign

@yubiuser yubiuser requested a review from a team April 11, 2022 19:30
rdwebdesign
rdwebdesign previously approved these changes Apr 12, 2022
Copy link
Member

@rdwebdesign rdwebdesign left a comment

Choose a reason for hiding this comment

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

Looks fine.

@yubiuser yubiuser requested a review from DL6ER April 12, 2022 20:12
api_FTL.php Outdated Show resolved Hide resolved
api_FTL.php Outdated Show resolved Hide resolved
@yubiuser yubiuser merged commit 47fcd87 into devel Apr 13, 2022
@yubiuser yubiuser deleted the speed/api branch April 13, 2022 16:40
@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-15-web-v5-12-and-core-v5-10-released/54987/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/confusion-from-the-main-display-screen/56102/4

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