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.

Sign up for GitHub

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

Jump to bottom

System.Net.WebProxy Bypass regex list optimization #73803

Merged
merged 3 commits into from
Aug 14, 2022
Merged

System.Net.WebProxy Bypass regex list optimization #73803

merged 3 commits into from
Aug 14, 2022

Conversation

onehourlate
Copy link
Contributor

Added change tracking to BypassArrayList to avoid reconstructing the bypass regex list at every call of IsBypassed

Fix #73703

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 11, 2022
@dnfadmin
Copy link

dnfadmin commented Aug 11, 2022 •
edited
Loading

CLA assistant check
All CLA requirements met.

@ghost
Copy link

ghost commented Aug 11, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Added change tracking to BypassArrayList to avoid reconstructing the bypass regex list at every call of IsBypassed

Fix #73703

Author: onehourlate
Assignees: -
Labels:

area-System.Net
Milestone: -

@onehourlate onehourlate mentioned this pull request Aug 11, 2022
Closed
stephentoub
stephentoub reviewed Aug 11, 2022
View reviewed changes
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
@stephentoub stephentoub mentioned this pull request Aug 11, 2022
Merged
stephentoub
stephentoub reviewed Aug 12, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than the comments, the change generally LGTM. Thanks for working on this.

src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated Show resolved Hide resolved
@stephentoub stephentoub added this to the 7.0.0 milestone Aug 12, 2022
Blaise Lepeuple and others added 3 commits August 14, 2022 07:58
@stephentoub
System.Net.WebProxy Bypass regex list optimization
f27b711 
Added change tracking to BypassArrayList to avoid reconstructing the bypass regex list at every call of IsBypassed

Fix dotnet#73703
@stephentoub
Cosmetic changes.
02b0c7a 
DirtyTrackingArrayList => ChangeTrackingArrayList.
IsDirty => IsChanged
Inner class moved to end
Ditched weird bool CheckDirty() with side effect in favor of void ResetIsChanged()
@stephentoub
Address PR feedback
c635195
@stephentoub
Copy link
Member

As we have a deadline of tomorrow, I've addressed the open feedback and pushed a new commit. Thanks.

@onehourlate
Copy link
Contributor Author

Ok.
Sorry, it wasn't clear to me you were waiting for more changes...
The fact that I didn't quite agree with the suggested changes didn't help ;)

I should have added all the possible mutation tests cases, tough : that's obviously a plus.

I don't like reflection in Unit Tests either, but I would have kept that test : with the current unit tests, the initial code would pass. I thought it was a net positive (despite the use of reflexion), to have unit tests that failed for the original code.

Anyway.
To be clear : do I still have something to do for this PR, or is it now going its merry way ?

@stephentoub
Copy link
Member

Sorry

No problem.

with the current unit tests, the initial code would pass

There are literally thousands of places things are cached around the core libraries, and except where there's a visible behavioral impact, we don't have unit tests for it. If you'd like to submit a PR to add a perf test for this to dotnet/performance, that'd be welcome.

do I still have something to do for this PR

Nope, thanks.

@stephentoub stephentoub merged commit 73ade4b into dotnet:main Aug 14, 2022
@stephentoub
Copy link
Member

Thank you, @onehourlate.

@MihaZupan MihaZupan mentioned this pull request Sep 6, 2022
Closed
@karelz karelz mentioned this pull request Sep 8, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@MihaZupan MihaZupan MihaZupan approved these changes

Assignees
No one assigned
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

System.Net.WebProxy Bypass regex list reconstructed for every call to IsBypassed
4 participants
@onehourlate @dnfadmin @stephentoub @MihaZupan