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

Optimize FieldTypesArray with static variable #367

Merged
merged 2 commits into from Nov 25, 2017

Conversation

joebordes
Copy link
Contributor

I was profiling my application which uses ADOdb and this optimization dropped my timing significantly, but I would like to ask if this change is valid.
The tests I made seemed to be working correctly.

@dregad
Copy link
Member

dregad commented Sep 18, 2017

Looks OK to me but if since this is about performance then you should probably use !empty($arr) instead of count($arr) > 0.

EDIT: actually, make that empty($arr) and move the loop within the if statement.

@mnewnham thoughts ?

@joebordes
Copy link
Contributor Author

Sounds correct to me. Have a look and let me know if you want me to change anything else.

Thanks!!

@mnewnham
Copy link
Contributor

Why not

static $arr = false;
if ($arr) 
    return $arr;

$arr = array();

blah blah blah....

@Mike-Benoit
Copy link
Contributor

Would be nice to get at least some idea of the actual performance improvement too. Is this a 1% improvement with a certain use case or 50%?

@dregad dregad added this to the v5.21 milestone Nov 25, 2017
@dregad dregad added core ADOdb core (library and base classes) enhancement labels Nov 25, 2017
@dregad dregad merged commit 6ec6a53 into ADOdb:master Nov 25, 2017
@Mike-Benoit
Copy link
Contributor

I think this just introduced another problem along the same lines of: #381.

Anytime data is cached, even in memory, there needs to be a way to clear it when schema changes and static variables make that job much more difficult.

@dregad
Copy link
Member

dregad commented Nov 25, 2017

Revert?

@Mike-Benoit
Copy link
Contributor

I would leave it in place for now, but I think we should figure out a better caching solution as part of #381.

@dregad
Copy link
Member

dregad commented Nov 25, 2017

OK.

@dregad
Copy link
Member

dregad commented Mar 8, 2021

I think this just introduced another problem along the same lines of: #381.

Anytime data is cached, even in memory, there needs to be a way to clear it when schema changes and static variables make that job much more difficult.

@Mike-Benoit it turns out I've just been hit by this... See #687

@peterdd
Copy link
Contributor

peterdd commented Mar 8, 2021

@dregad If you revert this you can move out

$max=$this->_numOfFields;

out of the loop I think. But probably just a micro optimization..

@dregad
Copy link
Member

dregad commented Mar 9, 2021

If you revert this you can move out

$max=$this->_numOfFields;

out of the loop I think. But probably just a micro optimization..

OK

Not yet sure if I'll revert or maybe try to find an alternative to using a static variable.

dregad added a commit that referenced this pull request Aug 15, 2021
Change the static array introduced in fieldTypesArray() (see #367) to a
class variable ($fieldObjectsCache), so the cache now only lasts for the
Recordset's lifetime.

Since the mssqlnative was already using a private $fieldObjects property
for the same purpose, the declaration was removed, and usages of the old
name changed to match the one defined in the parent class.

Fixes #687
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core ADOdb core (library and base classes) enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants