Skip to content

Reuse AstResolver memoizing on ClassLikeAstResolver#3498

Merged
samsonasik merged 3 commits intomainfrom
re-use-ast-resolver-in-class-like
Mar 21, 2023
Merged

Reuse AstResolver memoizing on ClassLikeAstResolver#3498
samsonasik merged 3 commits intomainfrom
re-use-ast-resolver-in-class-like

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik samsonasik commented Mar 21, 2023

@keulinho this is to re-use your memoizing on AstResolver already on ClassLikeAstResolver, also fix usage of parseFileNameToDecoratedNodes() that to be consistent to return array.

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.

@keulinho
Copy link
Copy Markdown
Contributor

keulinho commented Mar 21, 2023

LGTM 👍 I'll test drive this and try to get some blackfire profiles

@keulinho
Copy link
Copy Markdown
Contributor

In terms of performance improvements and memory usage this seems to be a micro-optimzation (at least in our test case, maybe there are cases where this improvement has a bigger impact)
You can see the comaparison here: https://blackfire.io/profiles/compare/a7ec9852-120b-4fb1-a6cc-f85d8bbf5eae/graph

But without the performance considerations, i like the fact to centralize the handling of the parsing, as that also makes further improvements easier 👍

@samsonasik
Copy link
Copy Markdown
Member Author

Yes, it mostly about consistency. Thank you for testing it, let's merge ;)

@samsonasik samsonasik merged commit fa4c805 into main Mar 21, 2023
@samsonasik samsonasik deleted the re-use-ast-resolver-in-class-like branch March 21, 2023 13:11
samsonasik added a commit that referenced this pull request May 8, 2023
* Reuse AstResolver memoizing on ClassLikeAstResolver

* fix return array

* fix return array
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.

2 participants