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

experimental source maps / improved cache layout #172

Closed

Conversation

o-alexandre-felipe
Copy link

This is the first step in the direction of enabling more powerful debugging

With this code generation and enabled tracing I am able to record the execution and then debug it step by step showing simultaneously the grammar location and the input location.

image

The change in the cache layout gives a significant speedup with a big grammar. On the benchmark repeating 100 times with cache, it is 3 times faster than the main branch.

Comparing my source-map and main branches

source-map --cache -n 100: 1417.90 kB/s
main --cache -n 100: 488.41 kB/s
source-map --cache -n 100: 2136.05 kB/s
main -n 100: 2122.96 kB/s

Next steps

There is some work to be done before we have proper source maps. But the idea is that the features on the page I use could be integrated in the code peggy language support. To allow step by step debugging, that would be a killer feature.
Also if it is interesting instead of recording the trace and replaying, to generate async rules, and traces calls that stop in endpoints and then can continue by resolving the promise returned by the trancer. With this approach we can have the debugging feature in the online demo page as well.

I also have a script that was an attempt to generate a javascript with a source map, however the source map is still buggy. Also for debugging in the browser step by step it is annoying because we have to give multiple steps to proceed to next step. If we have contribution of some guys with experience in generating source maps it will help.

Copy link
Member

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

I don't understand the whole idea how source-map support should be introduced by your changes. For now I think that my approach a much simpler and cleaner. Besides, you changed too much stuff that you shouldn't (space and style changes that even won't pass the lint). Sorry, but I can't accept PR in that state.

You also mixed several changes in one commit: source-map support stuff and cache stuff. Benchmark results looks interesting, but you didn't write how you run it. It is peggy benchmark?

If you like, you can extract cache-related changes to the new PR (or cleanup this PR and focusing only on that changes here), but don't forget to run npm run lint and npm test first!

@o-alexandre-felipe
Copy link
Author

o-alexandre-felipe commented Jul 8, 2021

Thank for the response and for providing the much better approach @Mingun
When are you releasing it?

Looking into your implementation I have a few doubts.

  1. The SourceNode will be able to map the statements when you give a block of code? (I imagined you would have to tokenize it before)
    https://github.com/Mingun/peggy/blob/55247d7d5d61192fa63cc11fcc9e4dd39e48329c/lib/compiler/passes/generate-js.js#L596
    push(new SourceNode(
      ast.topLevelInitializer.codeLocation.start.line,
      node.codeLocation.start.column - 1,
      options.grammarSource,
      ast.topLevelInitializer.code,
      node.type
    ));
  1. It seems that your function definition
function buildFunc(a, i) {
      return new SourceNode(
        null, null, options.grammarSource,
        [
          "\n  var " + f(i) + " = function(" + a.params.join(", ") + ") {",
          a.source,
          "};"
        ],
        f(i)
      );
    }

Will align incorrectly the action code with the beginning of the line (before var), the intention wasn't something like this?

new SourceNode(null, null, null, [
    "\n var " + f(i) + " = function(" a.params.join(", ") + ") {",
    new SourceNode(null, null, options.grammarSource, a.source),
    "};"
  ],
  a.source
); 
  1. I see that you are assigning source maps to the javascript (precisely the part I ignored in my solution), but where are you mapping the rests? I can't see where you set the source maps for Rules/Literals and classes (regular expressions).

@Mingun
Copy link
Member

Mingun commented Jul 8, 2021

The SourceNode will be able to map the statements when you give a block of code? (I imagined you would have to tokenize it before)

Yes, that's what it's designed for.

Will align incorrectly the action code with the beginning of the line (before var), the intention wasn't something like this?

As you can see in the regenerated parser.js, indentation is correct.

I can't see where you set the source maps for Rules/Literals and classes (regular expressions).

Only code blocks need source map because all other pieces of grammar you cannot debug.

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.

2 participants