addresses issue #310 #311
addresses issue #310 #311
Conversation
add a prettyprinter for NFAs Signed-off-by: Tim Bray <[email protected]>
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 96.32% 96.39% +0.06%
==========================================
Files 17 18 +1
Lines 1634 1718 +84
==========================================
+ Hits 1574 1656 +82
- Misses 34 35 +1
- Partials 26 27 +1 ☔ View full report in Codecov by Sentry. |
|
Once again, deep NFA twiddling, will push it myself if nobody wants to review. However, have a glance at TestPP() in prettyerinter_test.go to see what the prettyprinter output looks like for the wildcard pattern "x*9". I found this output absolutely essential in debugging the transition from DFA to NFA. In that output, the abbreviation If you have an idea for an improvement in the prettyerinter output, feel free to comment. |
There was a problem hiding this comment.
Left a comment regarding potential performance (allocation) issues. You should also update the README if you think this could be useful. For example, would it be good to enable printing behavior (for users) using an environment variable like Q_DEBUG_NFA to enable the printer?
Regarding printing output, might also be worth explaining that in README unless you want users/developers to only send a printer output file to you so you can debug?
| // printer is an interface used to generate representations of Quamina data structures to facilitate | ||
| // debugging and optimization. It's an interface rather than a type so that a null implementation can | ||
| // be provided for production that should incur very little performance cost. | ||
| type printer interface { |
There was a problem hiding this comment.
I'd suggest following what Google did when they developed the slog logging package. Instead of going with an interface type, they used a concrete struct with a specific interface handler type to avoid allocations: https://youtu.be/8rnI2xLrdeM?feature=shared&t=1336 (starts at that conversation)
There was a problem hiding this comment.
tl;dr
// inspired by https://pkg.go.dev/golang.org/x/exp/slog#Logger
type Handler interface {
Enabled() bool
Handle(...)
}
type Printer struct {
handler handler
}
func New() *Printer {...}There was a problem hiding this comment.
Hmm, looking at this. Unless you're actively testing/debugging and using newPrettyPrinter() the only allocation currently in production code is in value_matcher:
case shellStyleType:
newFA, nextField = makeShellStyleAutomaton(valBytes, &nullPrinter{})It dawns on me that if I put a global variable in pretty_printer.go like this:
var sharedNullPrinter = &nullPrinter{}Then that line in value_matcher becomes:
case shellStyleType:
newFA, nextField = makeShellStyleAutomaton(valBytes, sharedNullPrinter)Then there would be no allocations ever outside of when you're testing/debugging. Would that address your concern?
There was a problem hiding this comment.
Also, I agree about documenting this stuff. Probably best to create a DEVELOPING.md or some such; is there a standard name for this kind of thing?
There was a problem hiding this comment.
If this implementation is purely for developing, CONTRIBUTING.md might be common
There was a problem hiding this comment.
Left a comment in the place where I think alloc might happen? Ideally, we'd have a benchmark which shows the before/after here?
There was a problem hiding this comment.
Can't see alloc changes in this though: https://github.com/timbray/quamina/actions/runs/9324906941?pr=311
There was a problem hiding this comment.
Yes, CONTRIBUTING is the place.
There was a problem hiding this comment.
I added a description to CONTRIBUTING in the latest commit. If that's OK I think we can resolve this conversation?
Signed-off-by: Tim Bray <[email protected]>
| @@ -99,3 +105,52 @@ instructions for installing it. | |||
|
|
|||
| When opening a new issue, try to roughly follow the commit message format | |||
| conventions above. | |||
|
|
|||
| ## Developing | |||
There was a problem hiding this comment.
Great section! Since we talked about OS env vars, how would a dev enable the pretty printer easily? Would be nice to explain this here too.
There was a problem hiding this comment.
Good idea. CONTRIBUTING now contains a mini developer's user guide.
Signed-off-by: Tim Bray <[email protected]>
Thanks! Your input really improved this one. I'll let GitHub do the squashing, that seems to work OK with the CI. |
add a prettyprinter for NFAs