Skip to content

Conversation

@AtnNn
Copy link
Member

@AtnNn AtnNn commented Feb 2, 2017

This fixes #6202 for 2.3.x. #6203 fixes the issue in next.

Disabling the cache entirely is the safer option for a point release.

@AtnNn AtnNn added the tp:bug label Feb 2, 2017
@AtnNn AtnNn added this to the 2.3.6 milestone Feb 2, 2017
@VeXocide
Copy link
Member

VeXocide commented Feb 2, 2017

LGTM

@AtnNn AtnNn merged commit 5b833de into v2.3.x Feb 2, 2017
AtnNn added a commit that referenced this pull request Feb 2, 2017
@srh
Copy link
Contributor

srh commented Feb 2, 2017

This doesn't look good to me. How is this "safer"?

@srh
Copy link
Contributor

srh commented Feb 2, 2017

The changes in 4090d1c are smaller and very straightforward.

The regex cache exists for a reason. Throwing this performance regression into 2.3.6 is just broken.

@AtnNn
Copy link
Member Author

AtnNn commented Feb 2, 2017

This isn't a regression. The cache was never being hit (other than by empty strings). #6203 makes the cache work, which is a performance improvement, but exercises new code paths. That is why I called this a safer patch.

If I am wrong, then we should definitely use #6203 instead.

@srh
Copy link
Contributor

srh commented Feb 2, 2017

Oh I see. OK. LGTM!

AtnNn added a commit that referenced this pull request Feb 7, 2017
@srh srh deleted the atnnn/disable_regex_cache branch December 26, 2017 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants