Skip to content

Allow tracking of global/environment property reads#5038

Merged
rainersigwald merged 2 commits intodotnet:masterfrom
maneely:property_tracking_part_deux
Feb 25, 2020
Merged

Allow tracking of global/environment property reads#5038
rainersigwald merged 2 commits intodotnet:masterfrom
maneely:property_tracking_part_deux

Conversation

@maneely
Copy link
Copy Markdown
Contributor

@maneely maneely commented Jan 8, 2020

Re-reverting this as a starting point for #3432 and #5015

…#4461)" (#4663)"

This reverts commit faf5e5d.

@maneely maneely changed the title Revert "Revert "Adding tracking of properties with relevant events. (… [WIP] Revert "Revert "Adding tracking of properties with relevant events. (… Jan 9, 2020
@maneely maneely changed the title [WIP] Revert "Revert "Adding tracking of properties with relevant events. (… Revert "Revert "Adding tracking of properties with relevant events. (… Jan 9, 2020
@maneely
Copy link
Copy Markdown
Contributor Author

maneely commented Jan 9, 2020

This effectively adds back in the property tracking work done back in August '19 (with merge resolutions). This was reverted because of a failed RPS run with (IMO) questionable results.

Is it possible to get an RPS run before/after this change goes in (if accepted)?

@cdmihai
Copy link
Copy Markdown
Contributor

cdmihai commented Jan 9, 2020

Is it possible to get an RPS run before/after this change goes in (if accepted)?

Just wrote this a few days back: #4997 (comment)
Maybe I should add a piece of documentation on the current preferred way to of getting perf results.

For the OrchardCore case, you can repeat your measurements on master, your PR without property tracking, your PR with property tracking.

@rainersigwald rainersigwald added this to the MSBuild 16.6 Preview1 milestone Jan 14, 2020
@maneely
Copy link
Copy Markdown
Contributor Author

maneely commented Jan 15, 2020

Per @cdmihai, launched an experimental build to run VS perf tests with latest changes. It passed.

Forgind
Forgind approved these changes Jan 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

I didn't see you address Rainer's comment. Since it passed RPS, this otherwise looks good to me.

@rainersigwald rainersigwald changed the title Revert "Revert "Adding tracking of properties with relevant events. (… Allow tracking of global/environment property reads Feb 18, 2020
@rainersigwald rainersigwald removed the request for review from livarcocc February 18, 2020 18:10
rainersigwald
rainersigwald approved these changes Feb 18, 2020
View reviewed changes
@maneely @rainersigwald
Revert "Revert "Adding tracking of properties with relevant events. (d…
49987e7 
…otnet#4461)" (dotnet#4663)"

This reverts commit faf5e5d.

Fixing merge error in a comment.

Fix another merge issue.

Merge fix.
@maneely @rainersigwald
Make all property tracking opt-in
6426e79 
Property reassignments behave like before (on by default).
@rainersigwald rainersigwald force-pushed the property_tracking_part_deux branch from e981c55 to 6426e79 Compare February 25, 2020 16:21
@rainersigwald
Copy link
Copy Markdown
Member

rainersigwald commented Feb 25, 2020

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Feb 25, 2020

Azure Pipelines successfully started running 1 pipeline(s).

cdmihai
cdmihai reviewed Feb 25, 2020
View reviewed changes
@rainersigwald rainersigwald merged commit 5cb6772 into dotnet:master Feb 25, 2020
@rainersigwald rainersigwald mentioned this pull request Feb 25, 2020
Open
@rainersigwald rainersigwald mentioned this pull request Jul 2, 2020
Open
@KirillOsenkov KirillOsenkov mentioned this pull request Mar 31, 2021
Open
@rainersigwald rainersigwald mentioned this pull request Mar 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rainersigwald rainersigwald rainersigwald approved these changes

@AndyGerlicher AndyGerlicher Awaiting requested review from AndyGerlicher

+2 more reviewers

@cdmihai cdmihai cdmihai approved these changes

@Forgind Forgind Forgind approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

MSBuild 16.6 Preview1

Development

Successfully merging this pull request may close these issues.

4 participants

@maneely @cdmihai @rainersigwald @Forgind