Optimize size of Concerto parser

[accordproject]

Optimize size of Concerto parser

[mttrbrts]

Thanks @j4m3sb0mb, the speed vs size tradeoffs aren't well understood for this library as far as I know.

The last time that I looked at this, I saw a similar improvement in compile-time for the speed optimized version, which is great for CLI use and online-use (post-loading). However, the tradeoff is slower page loads for online-use and larger bundles when using concerto as a node dependency.

Perhaps this question helps?

Is an extra 100ms acceptable for Template Studio recompiles between edits in exchange for a 75% reduction in bundle size?

My previous assumption was that speed is more important.

Unless there is a clear consensus on this question, I suggest instead focussing on the removal of unused rules from this file (there are a lot of them!), https://github.com/accordproject/concerto/blob/master/packages/concerto-core/lib/introspect/parser.pegjs

That way we get a size reduction but still optimize for speed. 🏆

[j4m3sb0mb]

@mttrbrts
I've tried to remove all lines from 540 to 1260, it doesn't break the parser and it pass all tests.
parser.js size is then 356,2kb with speed optimization

[j4m3sb0mb]

I've tried to modify the script that creates the parser, with optimize option set to size the size of parser.js is 132kb instead of 558kb

"prepare": "pegjs --optimize size ./lib/introspect/parser.pegjs"

[jeromesimeon]

I've tried to modify the script that creates the parser, with optimize option set to size the size of parser.js is 132kb instead of 558kb

"prepare": "pegjs --optimize size ./lib/introspect/parser.pegjs"

@j4m3sb0mb Nice! Does that optimize version pass the tests?

[j4m3sb0mb]

@jeromesimeon yes it does

[jeromesimeon]

Thanks @j4m3sb0mb It's a relatively significant improvement. I don't see why we wouldn't do this for the time being. @dselman is traveling, but I can review a PR if you make one.

I would still leave this issue open for the other reasons listed (clean up of parser itself, investigate other parser generator technologies).

Would you like me to assign this issue to you for now?

[j4m3sb0mb]

yes

[jeromesimeon]

@j4m3sb0mb

I have just seen this in the pegjs documentation:

--optimize
Selects between optimizing the generated parser for parsing speed (speed) or code size (size) (default: speed)

Do you have any experience with performance related to this?

I haven't done much testing with pegjs myself, so I'll probably want to get some feedback on this from either @dselman or @mttrbrts

[j4m3sb0mb]

@jeromesimeon no experience, sorry

[j4m3sb0mb]

@jeromesimeon
I'm testing size optimized version and speed optimized once against org.accordproject.trademark but I don't find significant differences:

speed optimized version compiles in go in
748, 726, 798
while size optimized compiles in
838, 869, 842

[jeromesimeon]

I'll add two cents since @mttrbrts 's review really helped me understand what the questions are. I think I agree that speed during use matters more, even if that means the page initially loads a little slower (not sure how much slower really it's not that bad in Template Studio for instance). This is a one-time cost compared to the constant run-time cost when parsing.

[mttrbrts]

To be a little more specific about parser.pegjs. This file was originally a copy of a parser for JavaScript which we added new rules to capture the structure for Concerto (The new rules are here, https://github.com/accordproject/concerto/blob/master/packages/concerto-core/lib/introspect/parser.pegjs#L1555).

However, most of the original rules are still there unused (for example more of sections A.3 and A.4, Lines 540-1260). However, simply removing these lines is likely to break other rules that depend on these lines.

So refactoring will require some incremental changes and lots of testing to remove unused code.
The good news is that we have excellent code coverage, so running unit tests should flag issues quicky.

[mttrbrts]

Excellent! That's a nice surprise.

I suspect that there are lots of other rules that aren't used too. For example:

• URI definition, is much more advanced than we need. I'm sure that that could be simplified.
• Tokens, L476
• Keywords, L210

Have some fun!

[jeromesimeon]

Excellent! That's a nice surprise.

I suspect that there are lots of other rules that aren't used too. For example:

• URI definition, is much more advanced than we need. I'm sure that that could be simplified.
• Tokens, L476
• Keywords, L210

Have some fun!

Did a bit more in #154

The current parser looks pretty clean now. Tokens and Keywords have been cleaned up.

I'm unclear on the definition of what "more advanced than we need" is for URIs. Shouldn't we comply with some kind of URI spec? What would we remove?

Another area that might be simplified are Unicode character sets, but that's the same questions what do we believe the behaviour should be?

[mttrbrts]

@dselman @jeromesimeon It appears that pegjs is no-longer maintained pegjs/pegjs#667

There is a fork (https://github.com/peggyjs/peggy), however it is fairly nascent.

It this the opportunity to switch to a new technology entirely, perhaps standardise with a parser lib that we use in ergo or markdown-transform

[jeromesimeon]

@dselman @jeromesimeon It appears that pegjs is no-longer maintained pegjs/pegjs#667

There is a fork (https://github.com/peggyjs/peggy), however it is fairly nascent.

It this the opportunity to switch to a new technology entirely, perhaps standardise with a parser lib that we use in ergo or markdown-transform

Markdown transform uses: https://github.com/jneen/parsimmon

Ergo uses: http://gallium.inria.fr/~fpottier/menhir/

[mttrbrts]

This issue is partially address by the introduction of the new concerto-cto package which makes the parser an optional dependency. Closing this issue until there is new context for reviewing it again.