Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

count optimization #37

Merged
merged 1 commit into from
Aug 29, 2018
Merged

Conversation

samsonasik
Copy link
Contributor

  • Is this related to quality assurance?

@tux-rampage tux-rampage self-assigned this Jul 9, 2018
@tux-rampage tux-rampage added this to the 3.1.0 milestone Jul 9, 2018
Copy link
Contributor

@tux-rampage tux-rampage left a comment

Choose a reason for hiding this comment

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

Hi @samsonasik
thanks a lot for your contribution. I just added a minor change - note. But nothing critical, I could do this on merge. If you are able to change this, I'd appreciate.

@@ -134,7 +134,7 @@ private function buildParametersCode(string $type)
$tab = str_repeat(' ', $intention);
$code = '';

if (count($withOptions)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be imported with use function for performance. You may actively do this, but don't worry, I can do this during the pull as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

truthy check is faster than count($var)

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't make a difference with the current opcode optimisations in the language, but yes, a direct truth check is faster indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Misread of the diff on my side never mind.

Copy link
Member

@Xerkus Xerkus Sep 29, 2018

Choose a reason for hiding this comment

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

@Ocramius VLD opcodes for 7.1 show that count() actually does function call, initializing function context is the most expensive op here:

line     #* E I O op                           fetch          ext  return  operands
-------------------------------------------------------------------------------------
if ($var) :
   5     6      > JMPZ                                                     !0, ->8
   6     7    >   ECHO                                                     1
if (count($var)) :
   9     8    >   INIT_FCALL                                               'count'
         9        SEND_VAR                                                 !0
        10        DO_ICALL                                         $4      
        11      > JMPZ                                                     $4, ->13
  10    12    >   ECHO                                                     2
if (! empty($var)) :
  13    13    >   ISSET_ISEMPTY_VAR                           293601280  ~5      !0
        14        BOOL_NOT                                         ~6      ~5
        15      > JMPZ                                                     ~6, ->17
  14    16    >   ECHO                                                     3

Not that it really matters, it is optimized in 7.2

@tux-rampage tux-rampage requested a review from a team July 9, 2018 16:34
@tux-rampage tux-rampage modified the milestones: 3.1.0, 3.0.1 Jul 9, 2018
@Ocramius
Copy link
Member

Ocramius commented Jul 9, 2018

@samsonasik one note though: you keep sending these against master, but these are improvements, not fixes :-\

@samsonasik samsonasik changed the base branch from master to develop July 9, 2018 16:51
@samsonasik
Copy link
Contributor Author

@Ocramius rebased against develop and set target to develop

@tux-rampage
Copy link
Contributor

@Ocramius right, thanks.

@tux-rampage tux-rampage modified the milestones: 3.0.1, 3.1.0 Jul 9, 2018
@tux-rampage tux-rampage merged commit 4434894 into zendframework:develop Aug 29, 2018
tux-rampage added a commit that referenced this pull request Aug 29, 2018
tux-rampage added a commit that referenced this pull request Aug 29, 2018
@samsonasik samsonasik deleted the count-op branch August 29, 2018 11:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants