Skip to content
/ rector-src Public

[1.0] Replace NodeConnectingVisitor with ParentConnectingVisitor#3900

Merged
TomasVotruba merged 5 commits intomainfrom
replace
May 22, 2023
Merged

[1.0] Replace NodeConnectingVisitor with ParentConnectingVisitor#3900
TomasVotruba merged 5 commits intomainfrom
replace

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik samsonasik commented May 19, 2023

@TomasVotruba @staabm parent attribute can be reduced, but seems can't be removed completely to resolve Scope.

This to be ParentConnectingVisitor and remove NodeConnectingVisitor when all prev and next node attribute removed.

@samsonasik samsonasik requested a review from TomasVotruba as a code owner May 19, 2023 20:43
@samsonasik samsonasik marked this pull request as draft May 19, 2023 20:43
@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented May 19, 2023

using [ci skip] commit message to skip the rectify for now, while wait for all next/prev node usage removed.

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented May 21, 2023 •
edited
Loading

@TomasVotruba This require PR:

to be merged first for last next attribute usage removal, then it needs to be rebased.

@TomasVotruba TomasVotruba marked this pull request as ready for review May 22, 2023 08:00
@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented May 22, 2023

Ref rectorphp/rector#7854

@staabm
Copy link
Copy Markdown
Contributor

staabm commented May 22, 2023

I guess it would make sense to make some before/after measurements regarding memory and time for this PR

staabm
staabm reviewed May 22, 2023
View reviewed changes
packages/NodeTypeResolver/Node/AttributeKey.php
public const PARENT_NODE = 'parent';

/**
* @api
Copy link
Copy Markdown
Contributor

@staabm staabm May 22, 2023

Choose a reason for hiding this comment

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

even if the keys exist the attributes will no longer exist right?
do we need some kind of "bc preserving" mode which registers the NodeConnectingVisitor instead of ParentConnectingVisitor ?

in phpstan this is implemented via bleeding-edge

Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba May 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is rather to keep changes as small as possible. First replace logic, then remove constants in next PR.

@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented May 22, 2023 •
edited
Loading

@staabm Could you check the metrics before and after this PR e.g .on bin/rector process rules? I'm not familiar with it

@staabm
Copy link
Copy Markdown
Contributor

staabm commented May 22, 2023 •
edited
Loading

sure. just go ahead - I can do it even after the merge.

@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented May 22, 2023

@staabm Thank you 👍 Let's ship it!

Thanks @samsonasik for handling the upgrade path step step.

@TomasVotruba TomasVotruba merged commit 71f00d9 into main May 22, 2023
@TomasVotruba TomasVotruba deleted the replace branch May 22, 2023 08:36
@staabm
Copy link
Copy Markdown
Contributor

staabm commented May 22, 2023 •
edited
Loading

I did some measues in disabled parallel (for easier reproducability; more meaningful results) on the following command: time php bin/rector --dry-run --clear-cache --debug -vvv (running rector on its own codebase)

before

Run1:

	Peak-Memory: 4654 MB


	 [OK] 6 files would have changed (dry-run) by Rector


	real    6m21.391s
	user    0m0.046s
	sys     0m0.171s
	
	
Run2:
	Peak-Memory: 4650 MB


	[OK] 6 files would have changed (dry-run) by Rector



	real    6m23.570s
	user    0m0.015s
	sys     0m0.171s

after

Run1:

	Peak-Memory: 4542 MB


	 [OK] 6 files would have changed (dry-run) by Rector



	real    5m58.470s
	user    0m0.015s
	sys     0m0.062s

Run2:

	Peak-Memory: 4538 MB


	 [OK] 6 files would have changed (dry-run) by Rector



	real    5m59.116s
	user    0m0.031s
	sys     0m0.062s

My interpretation

  • we got 25-30 secs faster
  • we save only 100MB memory

-> I think garbage collection is still hurt by the parent-node attributes, as I would expect way less memory when our optimization really make garbage collection more efficient

clxmstaab pushed a commit to staabm/rector-src that referenced this pull request May 22, 2023
@staabm
Adjusted config to ease reproducing benchmarks
125d632 
run it with
`time php bin/rector --dry-run --clear-cache --debug -vvv`

compare results with rectorphp#3900 (comment)
clxmstaab pushed a commit to staabm/rector-src that referenced this pull request Jun 2, 2023
@staabm
Adjusted config to ease reproducing benchmarks
c0794cd 
run it with
`time php bin/rector --dry-run --clear-cache --debug -vvv`

compare results with rectorphp#3900 (comment)
staabm added a commit to staabm/rector-src that referenced this pull request Jun 10, 2023
@staabm
Adjusted config to ease reproducing benchmarks
a395025 
run it with
`time php bin/rector --dry-run --clear-cache --debug -vvv`

compare results with rectorphp#3900 (comment)
staabm added a commit to staabm/rector-src that referenced this pull request Jun 10, 2023
@staabm
Adjusted config to ease reproducing benchmarks
bf4cbbb 
run it with
`time php bin/rector --dry-run --clear-cache --debug -vvv`

compare results with rectorphp#3900 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomasVotruba TomasVotruba Awaiting requested review from TomasVotruba

1 more reviewer

@staabm staabm staabm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@samsonasik @TomasVotruba @staabm