Optimize silent fails: remove checks that always false

[Mingun]

The first сommit removed unnecessary checks with peg$silentFails if earlier in the parse function for the rule this variable was incremented. The second commit extends optimization for boundaries of rules: if rule always used in context of suppressed failure generation (there three of such context -- rules under named, simple_and and simple_not AST nodes) checks aren't generated.

The size of the generated parsers slightly decreased, and speed, most likely, didn't change though it would be worth expecting small improving. Measurements yield contradictory results, here the most noticeable of them with deviations in both sides (from 7 carried-out tests, in remaining distinction sticks near +/-0.5%, master is 48f5ea4):

$ ./impact master optimize-silent-fails
Measuring commit master... OK
Measuring commit optimize-silent-fails... OK

Speed impact
------------
Before:     164.95 kB/s
After:      141.32 kB/s
Difference: -14.33%

Size impact
-----------
Before:     721705 b
After:      646518 b
Difference: -10.42%

(Measured by /tools/impact with Node.js v0.10.18 on MINGW32_NT-6.0 1.0.18(0.48/3/2) i686.)

$ ./impact master optimize-silent-fails
Measuring commit master... OK
Measuring commit optimize-silent-fails... OK

Speed impact
------------
Before:     152.07 kB/s
After:      173.18 kB/s
Difference: 13.88%

Size impact
-----------
Before:     721705 b
After:      646518 b
Difference: -10.42%

(Measured by /tools/impact with Node.js v0.10.18 on MINGW32_NT-6.0 1.0.18(0.48/3/2) i686.)

[Mingun]

Last measurement show, that speed impact equals 1-4% (master is 5160235, optimize-silent-fails is b2fd6a8):

$ ./impact master optimize-silent-fails
Measuring commit master... OK
Measuring commit optimize-silent-fails... OK

Speed impact
------------
Before:     544.60 kB/s
After:      556.25 kB/s
Difference: 2.13%

Size impact
-----------
Before:     669581 b
After:      641730 b
Difference: -4.16%

(Measured by /tools/impact with Node.js v4.5.0 on MINGW64_NT-6.1 2.3.0(0.291/5/3) x86_64.)

[futagoza]

Is there a reason for this? ast.rules.length is always 1 or more, and the default value for options.allowedStartRules is [ast.rules[0].name], so there shouldn't be a need for this.

[Mingun]

ast.rules.length is always 1 or more

parse contract not guarantee it. Grammar without any rules still valid and can be occured in tests. The same reason for which all this if block in general exists -- tests are the special code demanding the special relation.

[futagoza]

What do you mean the parse contract doesn't guarantee it? The parser currently generated from the grammar will throw an error when a grammar without any rules is passed to it, so it is guaranteed that there will always be one rule from the PEG.js parser.

As I could not find a test case that ensured empty grammars are not accepted by the parser, I added one on my development-overhaul branch.

As for the compiler option allowedStartRules, since there will always be at least 1 rule, you can always expect it to be set as the compiler option default for allowedStartRules, so there's no need to set this, even in the test's, unless you are intentionally passing a rule name that doesn't exist to ensure the compiler throws an error.

[Mingun]

The parser currently generated from the grammar will throw an error when a grammar without any rules is passed to it

This is implementation detail. There are no reasonable reasons to do it. And if it changes in the future, tests which do not depend on it shall not fall. To test in the assumption as the code works means that you have no tests.

so there's no need to set this, even in the test's,

As it is possible to see few lines above, the options can be not supplied to the test and then it is meant empty. Thus, malformed data which to it shall not be supplied are obviously supplied to pass. Whether means it that the test in principle is totally wrong because it tries to expect correct behavior from a code, supply it certainly malformed data?

From this point of view this code only adds reasonable defaults to a test code, without forcing the developer of tests to supply their every time. Such simplification of operation is also required from the utility method, you do not agree?

[futagoza]

Since I do plan to add module support (#523) and annotations, it is likely in the future that there will be grammar files without rules, and you are correct that having this in the helper could actually be a plus for any developers making tests, so I accept this change, but can you extract this into a separate PR as this is a separate issue from this PR.

While you're at it, could you also in this new PR remove the related if ( options.allowedStartRules ) in lib/compiler/passes/report-undefined-rules.js#L22

If you don't want to make the new PR, just tell me and remove this code snippet from this PR, then I'll do the new PR at a later date.

[Mingun]

OK, now changes in the test helper method done in the #541 and this rebased on top of that. I also rewrote some parts (bugfix for not handling group node, (re)writed some comments) for bigger clarity.

[Mingun]

This pass is not extracted in the separate file because it strongly influences generation of a bytecode, and therefore shall be tested together with generation of a bytecode. Actually, it is just a detail of implementation of the bytecode generator

[Mingun]

This is added because I noticed, that for group node I forgot to update my PR for it, but tests did not tell me about that.

[coveralls]

Coverage increased (+1.4%) to 79.065% when pulling bf82874 on Mingun:optimize-silent-fails into a02f809 on pegjs:master.

[coveralls]

Coverage increased (+1.4%) to 79.065% when pulling bf82874 on Mingun:optimize-silent-fails into a02f809 on pegjs:master.

[futagoza]

ready to merge?

[Mingun]

Wait some time. I prepare the next PR with attempt to partially fix #194. It seems, it clashes with these changes

[Mingun]

Now it is ready to merge

[coveralls]

Coverage increased (+0.3%) to 75.583% when pulling ee47279 on Mingun:optimize-silent-fails into a7a0a0d on pegjs:master.