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

Use isset instead of array_key_exists #31

Merged
merged 1 commit into from Jan 18, 2015

Conversation

Projects
None yet
5 participants
@LukasReschke
Copy link
Contributor

LukasReschke commented Jan 17, 2015

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 17, 2015

Coverage Status

Coverage remained the same when pulling 35742c6 on LukasReschke:use-direct-array-key-access into 5b82f00 on punic:master.

@LukasReschke

This comment has been minimized.

Copy link
Contributor Author

LukasReschke commented Jan 17, 2015

This gives about 600ms for 100'000 invocations. May be more on slower machines etc…

@Remo

This comment has been minimized.

Copy link
Contributor

Remo commented Jan 18, 2015

is this about Punic\Calendar::formatDatetime or do you have any other performance issues at the moment? Might be helpful to have a benchmark script in the repo to run such tests. Performance was never an issue in my projects but I hardly ever have more than 10 punic calls per request..

Remo added a commit that referenced this pull request Jan 18, 2015

Merge pull request #31 from LukasReschke/use-direct-array-key-access
Use isset instead of array_key_exists

@Remo Remo merged commit 8cd28e7 into punic:master Jan 18, 2015

2 checks passed

Scrutinizer 1 new issues
Details
continuous-integration/travis-ci The Travis CI build passed
Details
@Remo

This comment has been minimized.

Copy link
Contributor

Remo commented Jan 18, 2015

@LukasReschke I'm trying to collect things we can improve over here, maybe you have some ideas / suggestions too #32

@mlocati

This comment has been minimized.

Copy link
Member

mlocati commented Jan 18, 2015

O(n) vs O(1)

PS: @LukasReschke: Isn't $array[$key] an O(n) ?

@LukasReschke LukasReschke deleted the LukasReschke:use-direct-array-key-access branch Jan 18, 2015

@LukasReschke

This comment has been minimized.

Copy link
Contributor Author

LukasReschke commented Jan 18, 2015

is this about Punic\Calendar::formatDatetime or do you have any other performance issues at the moment?

That's what we use at the moment – yes. My main concern is that a simple conversion of about 20k dates takes multiple seconds, for our use-case this is very undesirable.

Isn't $array[$key] an O(n) ?

Generally speaking $foo[$key] is a direct key access which is O(1), you can btw. also use it on strings to determine it's length, instead of counting the length, this makes a considerable difference as well especially with longer strings. Furthermore isset() is a language construct and so way faster if called multiple times than other non-language construct equivalents.

@stof

This comment has been minimized.

Copy link

stof commented Jan 21, 2015

2 things here:

  • both code are not equivalent (isset returns false if the key exists but contains a null value), so changes must be evaluated carefully (it is safe in many places, but not always)
  • are you profiling with XDebug enabled ? XDebug is known to slow down the function calls a lot. the difference between array_key_exists and isset is much bigger with XDebug than without it (which is also why XDebug should never be enabled on prod servers)
@LukasReschke

This comment has been minimized.

Copy link
Contributor Author

LukasReschke commented Jan 21, 2015

are you profiling with XDebug enabled ? XDebug is known to slow down the function calls a lot. the difference between array_key_exists and isset is much bigger with XDebug than without it (which is also why XDebug should never be enabled on prod servers)

XDebug is disabled on my local machine ;-) (at least I comment it out pretty often)

@mlocati

This comment has been minimized.

Copy link
Member

mlocati commented Jan 21, 2015

both code are not equivalent (isset returns false if the key exists but contains a null value), so changes must be evaluated carefully (it is safe in many places, but not always)

For that exact reason, before accepting this PR I double checked every change that @LukasReschke did, and all were correct 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment