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

Pre-generate builtins and keywords for lexers #1418

Merged
merged 8 commits into from Apr 14, 2020

Conversation

ashmaroli
Copy link
Contributor

Instead of loading a YAML file and computing the sets from the data at runtime, to reduce memory usage.

@ashmaroli
Copy link
Contributor Author

Memory profile summary

--- master https://travis-ci.org/rouge-ruby/rouge/jobs/642229408
+++ PR     https://travis-ci.org/rouge-ruby/rouge/jobs/643419479

- Total allocated: 14.70 MB (142309 objects)
+ Total allocated: 13.53 MB (132177 objects)
- Total retained:  5.31 MB (37531 objects)
+ Total retained:  5.09 MB (35512 objects)

@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Feb 1, 2020
@pyrmont pyrmont self-assigned this Feb 1, 2020
@pyrmont
Copy link
Contributor

pyrmont commented Feb 1, 2020

@ashmaroli Thanks for this—that looks like a nice little reduction. Do you see any issues, @jneen?

@pyrmont pyrmont added maintainer-action The PR has been reviewed but action by a maintainer is required and removed needs-review The PR needs to be reviewed labels Feb 1, 2020
@ashmaroli
Copy link
Contributor Author

@jneen Requesting your feedback on this so that it can be included in the upcoming release.
Thank you.

@ashmaroli
Copy link
Contributor Author

Will this get included in the upcoming release..?

@pyrmont
Copy link
Contributor

pyrmont commented Apr 2, 2020

@ashmaroli Sorry, still waiting for @jneen's feedback.

@ashmaroli
Copy link
Contributor Author

Memory profile summary

--- master https://travis-ci.com/github/rouge-ruby/rouge/jobs/318551844
+++ PR     https://travis-ci.com/github/rouge-ruby/rouge/jobs/318777292

- Total allocated: 14.14 MB (141625 objects)
+ Total allocated: 13.07 MB (132473 objects)
- Total retained:  5.44 MB (37843 objects)
+ Total retained:  5.22 MB (35959 objects)

@pyrmont
Copy link
Contributor

pyrmont commented Apr 14, 2020

@ashmaroli I think this has been in review for long enough. I'm happy to merge this but is there anything else you wanted to add or is it ready from your side, too?

@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed maintainer-action The PR has been reviewed but action by a maintainer is required labels Apr 14, 2020
@pyrmont
Copy link
Contributor

pyrmont commented Apr 14, 2020

@ashmaroli It seemed like it would be handy to have a single task that runs all the builtin generators but in the course of testing it, I came across a bunch of bugs in some of the current Rake tasks. I've fixed those and generated new builtin files.

Perhaps I should revert the most recent commits and submit as a separate PR but I thought for the moment, I'd just build on your work here. What do you think?

@ashmaroli
Copy link
Contributor Author

@pyrmont I think its a good idea to have a One Task to rule generate them all.. 😉

But if the additional changes are going to push this PR to be included in the next month's release rather than today's, I feel it'd be better if the new changes were in a separate PR so that if I were asked to make changes to this proposal at a future date, then it'd be easier for me if I didn't have to wrap my head around your changes as well...

@pyrmont
Copy link
Contributor

pyrmont commented Apr 14, 2020

Yeah, fair enough. Will revert.

@pyrmont pyrmont merged commit e4b3d4f into rouge-ruby:master Apr 14, 2020
@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Apr 14, 2020
@pyrmont
Copy link
Contributor

pyrmont commented Apr 14, 2020

Thanks for the PR, @ashmaroli. I'll open a separate one for the generate task.

@ashmaroli
Copy link
Contributor Author

Thank you very much @pyrmont :)

@pyrmont pyrmont mentioned this pull request Apr 14, 2020
mattt pushed a commit to NSHipster/rouge that referenced this pull request May 21, 2020
Rouge has a number of lexers that include a list of the names of
built-in functions for more accurate tokenisation. Some of these lists
consist of YAML files that need to be computed at runtime. Rather than
take this approach, this commit ensures that the lists are plain Ruby
files that can be loaded by the respective lexers.
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