Skip to content

Fix bug in scanner cache PR when running kprove#2408

Merged
rv-jenkins merged 3 commits intomasterfrom
cache2
Jan 25, 2022
Merged

Fix bug in scanner cache PR when running kprove#2408
rv-jenkins merged 3 commits intomasterfrom
cache2

Conversation

@dwightguth
Copy link
Copy Markdown
Contributor

You should not need to review the first commit as it is identical to the one previously reviewed. This PR just fixes the issue where we were still trying to serialize the scanner when parsing rules in kprove.

Note that we do not need to fix the case where we are deserializing the scanner yet, because we do not yet use the serialized scanner while parsing anything other than search patterns. In a follow-up PR, once this is suitably working, we will need to ensure that we use the cached scanner during kprovex

Dwight Guth and others added 2 commits January 21, 2022 11:22
…ern` (#2390)

* fix bug where command line invocation was not printed with -v

* delete dead code

* code to serialize scanner

* implement Comparable on TerminalLike

* make tokens deterministic in numbering

* implement code to deserialize scanner

* add tests for bug

* fix #2372 - false negative of invalid precedence error

* fix bug involving syntax added by compiler to main module

* fix precedence conflict in test

* Fix indent

Co-authored-by: rv-jenkins <admin@runtimeverification.com>
@dwightguth dwightguth marked this pull request as ready for review January 24, 2022 18:06
Copy link
Copy Markdown
Contributor

@SchmErik SchmErik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@radumereuta
Copy link
Copy Markdown
Contributor

@dwightguth can you remind me how is this supposed to work?
I tested this and it still generates extra scanners even when kompiling twice.

regression-new/map-symbolic-tests-haskell$ kompile --no-exc-wrap --backend haskell  map-tests.k -d . -v
Parse command line options                                   =  0.011s
Importing: Source(/home/radu/work/k/k-distribution/tests/regression-new/map-symbolic-tests-haskell/map-tests.k)
Outer parsing [67 modules]                                   =  0.772s
  New scanner: MAP-TESTS-CONFIG-CELLS                        =  0.142s
  New scanner: STDOUT-STREAM-CONFIG-CELLS                    =  0.196s
  New scanner: STDIN-STREAM-CONFIG-CELLS                     =  0.220s
Parse configurations [3/3 declarations]                      =  0.525s
  New scanner: MAP-TESTS-RULE-CELLS                          =  0.128s
  New scanner: STRING-KORE-RULE-CELLS                        =  0.176s
  New scanner: STRING-COMMON-RULE-CELLS                      =  0.199s
  New scanner: MAP-TESTS-RULE-CELLS                          =  0.215s
  New scanner: K-IO-RULE-CELLS                               =  0.314s
  New scanner: STDOUT-STREAM-RULE-CELLS                      =  0.294s
  New scanner: STDIN-STREAM-RULE-CELLS                       =  0.322s
Parse rules [120/120 rules]                                  =  0.635s
Validate definition                                          =  0.149s
Apply compile pipeline                                       =  0.298s
Kompile to kore                                              =  0.005s
Save to disk                                                 =  0.033s
Backend                                                      =  0.342s
Cleanup                                                      =  0.001s
Total                                                        =  2.771s
regression-new/map-symbolic-tests-haskell$ kompile --no-exc-wrap --backend haskell  map-tests.k -d . -v
Parse command line options                                   =  0.011s
Importing: Source(/home/radu/work/k/k-distribution/tests/regression-new/map-symbolic-tests-haskell/map-tests.k)
Outer parsing [67 modules]                                   =  0.725s
Parse configurations [0/3 declarations]                      =  0.287s
  New scanner: MAP-TESTS-RULE-CELLS                          =  0.133s
Parse rules [0/120 rules]                                    =  0.189s
Validate definition                                          =  0.132s
Apply compile pipeline                                       =  0.346s
Kompile to kore                                              =  0.006s
Save to disk                                                 =  0.033s
Backend                                                      =  0.388s
Cleanup                                                      =  0.001s
Total                                                        =  2.118s
regression-new/map-symbolic-tests-haskell$ 

@dwightguth
Copy link
Copy Markdown
Contributor Author

The goal of this PR isn't to stop scanners from being created when you run kompile; that's because we don't have any logic for deciding if the cached scanner is out of date, nor would it be particularly easy to do this. It is merely to allow us to reuse scanners during krun and eventually kprovex that are identical to the one generated by kompile

@radumereuta
Copy link
Copy Markdown
Contributor

Ok, fine for now.
But you can check that the grammar used to generate the scanner is equal.
It's a coarse solution because you check the entire grammar instead of only the lexical part but it should work.

@dwightguth
Copy link
Copy Markdown
Contributor Author

Sure, in theory you can, but any substantive change to the definition's syntax will ultimately invalidate the scanner, whereas you have now introduced a new check for equality that is pretty massive that has to be run on every invocation of kompile. I'm not convinced it's really going to help much during day to day development. Besides, my primary goal with this change is to reduce time spent on the hot path, which doesn't include time spent in kompile. We can separately try to improve time spent in kompile, of course, but generating the scanner isn't generally where most of that time is being spent in large definitions right now. Regardless, I'm going to merge this and we can continue to iterate improvements

@radumereuta
Copy link
Copy Markdown
Contributor

radumereuta commented Jan 25, 2022

We do grammar comparison when we invalidate cached rules anyway.
Why are you saying this is 'massive'?

@rv-jenkins rv-jenkins merged commit ba37da2 into master Jan 25, 2022
@rv-jenkins rv-jenkins deleted the cache2 branch January 25, 2022 22:39
@dwightguth
Copy link
Copy Markdown
Contributor Author

We do grammar comparison when we invalidate cached rules if we have rules in the module in question. However, if the definition consists of many modules each with a few pieces of syntax, all imported into one module with no rules, then we won't actually need to do the cache invalidation check on the entire grammar all at once. Thus, you would still end up doing additional work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants