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

Moved the parsers into a hash table #25

Merged
merged 17 commits into from
Dec 31, 2016
Merged

Conversation

Witiko
Copy link
Collaborator

@Witiko Witiko commented Dec 27, 2016

This series of commits moves parsers into a separate hash table as
discussed in #24. The average CPU time in seconds measured by
make bench (500 samples) went up from 0.617380 to 0.632300.
This is likely due to the hash table access overhead as you predicted.

@jgm jgm merged commit 9e32050 into jgm:master Dec 31, 2016
@jgm
Copy link
Owner

jgm commented Dec 31, 2016

Thanks. That seems an acceptable performance loss!

@craigbarnes
Copy link

Wouldn't it make more sense to use an LPeg grammar table for this?

@Witiko
Copy link
Collaborator Author

Witiko commented Jan 24, 2017

Lunamark already uses a grammar table (see the syntax variable in
lunamark/reader/markdown.lua). However, it contains only the top-level rules.
The users can currently supply a function that makes modifications to the
syntax table (see the alter_syntax option in lunamark/reader/markdown.lua),
which arguably makes the top-level rules a part of the library interface. By
including all the helper patterns into the syntax table, we would be exposing
much more than necessary and any changes could break user code.

The helper table could be rewritten as an LPeg grammar table, although a couple
entries would need to be removed, since the table contains not only LPeg
patterns, but also a number of functions that return LPeg patterns. However,
what do you hope this would accomplish considering that the helper patterns are
non-recursive?

drehak pushed a commit to drehak/lunamark that referenced this pull request Jul 27, 2022
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.

3 participants