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

Simplify getVisitFn #694

Merged
merged 5 commits into from
Jul 21, 2020
Merged

Simplify getVisitFn #694

merged 5 commits into from
Jul 21, 2020

Conversation

shmax
Copy link
Contributor

@shmax shmax commented Jul 1, 2020

I noticed in the profiler that Visitor::getVisitFnseems to be consuming a lot of resources, so I attempted to simplify it a bit.

Here are the results of running our unit test suite 5 times.

This branch:
https://blackfire.io/profiles/de85207f-a7bc-4d69-bbc1-e950c839eb24/graph

Master:
https://blackfire.io/profiles/a1b07e4f-cba2-4327-84c5-c42e7559d8b2/graph

This is kind of an extreme use case, but you can see we improve performance by about 5%.

@coveralls
Copy link

coveralls commented Jul 1, 2020

Coverage Status

Coverage increased (+0.002%) to 86.323% when pulling 605d2df on shmax:optimize-getvisitfn into 09469da on webonyx:master.

@shmax
Copy link
Contributor Author

shmax commented Jul 2, 2020

Maybe you can help me with the coverage reduction. It's flagging this line as not covered...

https://coveralls.io/builds/31813089/source?filename=src/Language/Visitor.php#L234

...but it was not covered on master, either (I verified in the debugger). How do I handle this?

@shmax shmax changed the title simplify getVisitFn Simplify getVisitFn Jul 2, 2020
@spawnia
Copy link
Collaborator

spawnia commented Jul 8, 2020

@shmax since you simplified the function, one uncovered line now is worse in terms of percentage.

I don't think we use arrays in place of a NodeList anymore - maybe we never have? There are a few courses of action we could take:

  1. Write an explicit test to cover it (might involve suppressing PHPStan errors in the setup code)
  2. Ignore the uncovered line (we might be able to exclude it explicitly?)
  3. Remove the functionality (we might do that in a next major, though, and go with 1. or 2. for now)

@shmax
Copy link
Contributor Author

shmax commented Jul 8, 2020

I don't think we use arrays in place of a NodeList anymore - maybe we never have? There are a few courses of action we could take:

Ahhh, I see... does this have anything to do with that one fields member we saw on one of the nodes that was an array, but should probably be a NodeList? How about if I add a fix for that to this change set, and we just remove the dead code in a major version, like you say?

@shmax
Copy link
Contributor Author

shmax commented Jul 9, 2020

I went ahead and changed that fields member to a NodeList and removed that bit of dead code. I don't know if this will have any impact on the public API, but the tests and stan seem happy. Thank you very much for the guidance. 👍

@shmax
Copy link
Contributor Author

shmax commented Jul 10, 2020

@vladar What do ya say?

Copy link
Member

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@vladar vladar merged commit 3f66e38 into webonyx:master Jul 21, 2020
@shmax shmax deleted the optimize-getvisitfn branch July 21, 2020 11:07
@shmax
Copy link
Contributor Author

shmax commented Jul 21, 2020

@spawnia @vladar Thanks very much for reviewing, guys. 👍

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