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.

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

Implement re.replace(string, Regex, proc(...)) , simplify implmentation #1855

Closed
wants to merge 7 commits into from
Closed

Implement re.replace(string, Regex, proc(...)) , simplify implmentation #1855

wants to merge 7 commits into from

Conversation

flaviut
Copy link
Contributor

@flaviut flaviut commented Jan 4, 2015

Exactly what it says on the tin.

I'd like to sneak 1bfc93c in, but the real meat is in 0aba0f5.

@Araq
Copy link
Member

Araq commented Jan 18, 2015

flaviut: Your re.replace(string, Regex, proc(...)) is fine but simplify the implemenation by using a private template please to avoid performance regressions and to avoid some artefacts that effect analysis might produce (it shouldn't but maybe I overlooked something).

@flaviut
Copy link
Contributor Author

flaviut commented Jan 18, 2015

I've improved performance, but it's still not quite as good as before. I'm not really sure why.

IMO it's close enough, please give more feedback.

@Araq
Copy link
Member

Araq commented Feb 4, 2015

Well how much slower is it? 1% ?

flaviut and others added 7 commits February 20, 2015 20:46
The other replace methods can be cleaned up to be in terms of this
Use the recently-created implementation using a lambda
Instead of passing the full matches array to the arguments of the proc, it
passes only the matches that are possible
replacef tends to mean that it receives that captures
new: 11.95
old: 8.47
new template: 9.33
@flaviut
Copy link
Contributor Author

flaviut commented Feb 21, 2015

old: 8.47
new: 9.33

In percents, that's ~10% slower on a micro-benchmark. Not nearly as trivial as you hoped unfortunately. My profiler is having trouble understanding the code, so I've run into a wall optimizing.

I've lost the code to that benchmark, but

import re

const data = slurp("./rand") # 1K file of random bytes
let pattern = re"\d"

var optbuster = 0

for i in 1 .. 100000:
  optbuster += data.replacef(pattern, "asdasd")[5].int

echo optbuster

arrives at similar results

@Araq
Copy link
Member

Araq commented Mar 22, 2015

Do it properly then please.

@Araq Araq closed this Mar 22, 2015
@flaviut flaviut deleted the regex-feature branch May 26, 2015 13:48
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.

None yet

3 participants