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

Use static regex for simple codespan results #597

Closed
wants to merge 3 commits into from

Conversation

ashmaroli
Copy link
Contributor

@ashmaroli ashmaroli commented May 6, 2019

Rationale

It is wasteful to allocate /`/ and /`\Z/ every time a simple codespan is parsed. Instead use a static regex which is allocated only once each.

Context

Results from profiling Jekyll docs:

Before

Total allocated: 158.07 MB (1168216 objects)
Total retained:  10.3 MB (92547 objects)

After

Total allocated: 157.1 MB (1162835 objects)
Total retained:  10.3 MB (92547 objects)

@ashmaroli
Copy link
Contributor Author

@ashmaroli ashmaroli commented May 23, 2019

Profiler Diff Summary

--- master branch https://travis-ci.org/gettalong/kramdown/jobs/536192573
+++ PR branch     https://travis-ci.org/gettalong/kramdown/jobs/536323986

- Total allocated: 220.89 MB (1783975 objects)
+ Total allocated: 219.34 MB (1775370 objects)
  Total retained:  1.11 MB (1582 objects)

To avoid generating another set of regex and matchdata


Co-authored-by: Thomas Leitner <t_leitner@gmx.at>
@ashmaroli
Copy link
Contributor Author

@ashmaroli ashmaroli commented Jul 17, 2019

@gettalong I guess we were wrong. The latest change isn't synonymous with the existing behavior:

irb(main):001:0> string = "`foobar`\n"
=> "`foobar`\n"

irb(main):002:0> string.sub(/`\Z/, '')
=> "`foobar\n"

irb(main):003:0> string.chomp('`')
=> "`foobar`\n"

irb(main):004:0> string.sub(/`\Z/, '') == string.chomp('`')
=> false

Shall I revert the latest commit or am I testing incorrectly?

@gettalong
Copy link
Owner

@gettalong gettalong commented Jul 17, 2019

You are right, \Z matches before a new line at the end, \z matches at the end. I guess the original version it is.

@ashmaroli
Copy link
Contributor Author

@ashmaroli ashmaroli commented Feb 10, 2020

@gettalong Pinging you as a gentle reminder...

@gettalong
Copy link
Owner

@gettalong gettalong commented Feb 13, 2020

@ashmaroli Sorry for the delay, I will find some time in the coming days for kramdown.

@gettalong
Copy link
Owner

@gettalong gettalong commented Apr 17, 2020

@ashmaroli Even though the winnings are not much, the changes are not very invasive and I will therefore accept them.

@gettalong gettalong closed this Apr 17, 2020
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

2 participants