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

Don’t use fetchAll and enable initialization from array #1287

Merged
merged 7 commits into from Feb 14, 2020

Conversation

@ausi
Copy link
Member

ausi commented Feb 4, 2020

This is related to #1274 (comment) an implementation of the Result class that doesn’t use fetchAll and only copies the data for used/read rows.

But I think for the use case in #1274 we should enable the initialization from an array instead of the overwrite() method. I’ve added this already to this PR.

@ausi ausi added the defect label Feb 4, 2020
@ausi ausi added this to the 4.4 milestone Feb 4, 2020
@ausi ausi self-assigned this Feb 4, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 4, 2020

Nice work! 😎

we should enable the initialization from an array instead of the overwrite() method. I’ve added this already to this PR.

I think we should do neither, because both are API changes (albeit backwards compatible) in one of the central framework classes, which should not be made in a bugfix release. And with #1270 there is a working alternative, which might not be as clean as #1274 but at least does not introduce new public API methods.

@ausi

This comment has been minimized.

Copy link
Member Author

ausi commented Feb 4, 2020

I think we should do neither, because both are API changes

Agreed, so we use #1270 for version 4.4 and 4.9 and I rebase this PR to 4.10?

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 4, 2020

I would still merge the PR – except for the "initialize from array" part – in Contao 4.4, because it fixes the memory consumption of the Result class, doesn't it?

@ausi

This comment has been minimized.

Copy link
Member Author

ausi commented Feb 4, 2020

because it fixes the memory consumption of the Result class, doesn't it?

Yes, it improves memory usage IMO. But it is still not optimal because the Doctrine Statement doesn’t allow jumping back to a previous row.

My implementation in this PR only fetches from the DoctrineStatement if needed and stores it in the internal array. If/when all rows are fetched it closes and removes the DoctrineStatement (because then all rows are stored).

@ausi

This comment has been minimized.

Copy link
Member Author

ausi commented Feb 5, 2020

I would still merge the PR – except for the "initialize from array" part – in Contao 4.4

If we merge this PR in Contao 4.4 I think we can enable the "initialize from array" part in 4.9 already, as the code for the change is then very minimal and 4.9 was not yet released.

Copy link
Member

leofeyer left a comment

Actually, the PR does not change much, does it? We will still load the entire result set into memory when we walk through all rows – which is the 99% case.

Ideally, we would be able to remove protected $resultSet and only load the current record into protected $arrCache. Otherwise we might as well leave it as is. 🙈

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Feb 7, 2020

I don't think that's true? We load each row if it's used. If I foreach-loop over a result and break at some point, the remaining results are not loaded. I can fetch the first 100 items from a result with 1000 without loading everything into memory …

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 7, 2020

Yes, but that is the 1% case. In 99% of all cases, we are fetching all rows from the result.

@ausi

This comment has been minimized.

Copy link
Member Author

ausi commented Feb 7, 2020

but that is the 1% case. In 99% of all cases, we are fetching all rows from the result.

That might be true, but we now have the same performance for the 99% cases and a great improvement for the 1% case.

But thinking about performance, we could probably improve the cases for numRows === 1. I will add this to the PR.

@ausi ausi dismissed stale reviews from Toflar and aschempp via a29bd2c Feb 8, 2020
@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Feb 10, 2020

I don't really understand the changes now. How do you seek back in the result set if you don't cache it anymore?

@ausi

This comment has been minimized.

Copy link
Member Author

ausi commented Feb 10, 2020

All “loaded” rows are stored in resultSet and this array is used to seek back.

arrCache was just a cache for the current row and it’s modifications. IMO it makes more sense to just store the modifications and don’t copy every row into arrCache when it is loaded.

Copy link
Member

leofeyer left a comment

I am approving the PR, because the implementation is correct. However, the changes are not solving the initial problem at all. 🙈

@ausi

This comment has been minimized.

Copy link
Member Author

ausi commented Feb 10, 2020

IMO we should use #1270 for Contao 4.4 and implement the array initialization for Contao 4.9.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 13, 2020

As discussed in Mumble on February 13th, we want to merge this PR as is to fix the bugs in the Result class. Then in Contao 4.9, we want to add array initialization and merge #1274.

@leofeyer leofeyer merged commit b05d664 into 4.4 Feb 14, 2020
10 checks passed
10 checks passed
Coverage
Details
Coding Style
Details
PHP 5.6
Details
PHP 7.1
Details
PHP 7.2
Details
PHP 7.3
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project 88.51% (+0.00%) compared to ca47554
Details
@leofeyer leofeyer deleted the bugfix/result-without-fetchall branch Feb 14, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 14, 2020

Thank you @ausi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.