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

proposal: standardize header size prepended before executing scripts in v8 #17256

Closed
bcoe opened this issue Nov 22, 2017 · 6 comments
Closed
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem.

Comments

@bcoe
Copy link
Contributor

bcoe commented Nov 22, 2017

  • Version all:
  • Platform all:
  • Subsystem: coverage,testing:

Problem

I've been working with folks on the v8 team to try to move towards using v8's built in coverage as a viable way to collect coverage for Node.js scripts outside of the browser.

When a common-js module is executed in v8, the script is wrapped in a closure which introduces 62 additional bytes to the first line of the application. When an ES module is executed I believe it introduces a different byte count (there's actually no wrapper?).

It's difficult for tooling that instruments JavaScript source to predict what header might have been inserted on a source file before it was executed in v8; This leads to inaccurate stack traces and coverage reports.

Goals

  • Accurate stack-traces and test coverage tracking, regardless of whether a module is common-js or ESM.
  • A developer writing tooling should not have to know what path a module took through the loader.

Potential Solution

  • standardize on a padding size that will be inserted on the first line of a script executed in v8 (e.g., 128 characters); ESM or common-js should pad to this size with a no-op, e.g., throw a bunch of spaces on the end of the header.
  • expose this padding size as a global variable so that developers writing tooling can reference this value, rather than hardcoding a value.
  • it would also be good to do something similar for shebang handling.

CC: @chrisdickinson, @bmeck, @schuay, @ak239

@bmeck
Copy link
Member

bmeck commented Nov 22, 2017

For shebangs, it has been added to TC39 agenda under "InterpreterDirective" to see if it can be incorporated into the grammars.

Another solution is potentially to just put source maps in all files that are manipulated via source text manipulation by node I think.

Is there a reason it needs to be a number/byte count?

@bcoe
Copy link
Contributor Author

bcoe commented Nov 22, 2017

Another solution is potentially to just put source maps in all files that are manipulated via source text manipulation by node I think.

@schuay or @ak239 can better speak to how viable this solution would be -- my main concern would be in whether or not we're running coverage through source-map handling at a point in the stack where it would be easy to take advantage of this information. tldr; I'd love to see v8 test-coverage be a viable solution in Node.js in a reasonable time frame.

@bnoordhuis
Copy link
Member

Would passing -62 as the column offset to v8::ScriptOrigin (and thence to v8::ScriptCompiler::CompileUnbound()) work?

I remember trying that some years ago and I believe it worked okay except for a bug in the old debugger (which is gone now.)

@mscdex mscdex added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. feature request Issues that request new features to be added to Node.js. labels Nov 22, 2017
@schuay
Copy link

schuay commented Nov 23, 2017

I think we should fix this in V8. The problem is that we report source positions without taking wrapper script offsets into account. We should have access to these in Script::line_offset and Script::column_offset here. Opened https://crbug.com/v8/7119.

inaccurate stack traces

Just double-checking - stack traces should display the correct line/column positions, don't they?

cc @hashseed

@bcoe
Copy link
Contributor Author

bcoe commented Nov 27, 2017

@bnoordhuis thanks for helping point us in the right direction, we have some work in progress now that applies column/line offset to the coverage positions.

Off the top of your head, should these values being set appropriately when a script is currently executed in v8? I was reading this comment and didn't quite follow the current state of affairs.

@bcoe
Copy link
Contributor Author

bcoe commented Nov 30, 2017

conversation has moved to #17396

@bcoe bcoe closed this as completed Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants