Source map support #2082

mstoykov · 2021-07-02T15:00:58Z

No description provided.

mstoykov · 2021-07-02T15:03:47Z

This basically fixes #1789 , although there is still more work to be done especially given that this has zero automatic testing. Also, I haven't done any archive testing.

codecov-commenter · 2021-07-02T15:09:02Z

Codecov Report

Merging #2082 (3966dc8) into master (aa1fd6a) will decrease coverage by 0.16%.
The diff coverage is 44.87%.

❗ Current head 3966dc8 differs from pull request most recent head 951fcba. Consider uploading reports for the commit 951fcba to get more accurate results

@@            Coverage Diff             @@
##           master    #2082      +/-   ##
==========================================
- Coverage   72.11%   71.95%   -0.17%     
==========================================
  Files         180      180              
  Lines       14243    14297      +54     
==========================================
+ Hits        10272    10287      +15     
- Misses       3346     3380      +34     
- Partials      625      630       +5

Flag	Coverage Δ
ubuntu	`71.89% <44.87%> (-0.16%)`	⬇️
windows	`71.59% <44.87%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/runtime_options.go	`100.00% <ø> (ø)`
js/initcontext.go	`87.75% <16.66%> (-4.80%)`	⬇️
js/bundle.go	`85.47% <42.85%> (-8.08%)`	⬇️
js/compiler/compiler.go	`58.44% <48.71%> (-7.63%)`	⬇️
cmd/runtime_options.go	`85.50% <50.00%> (-2.19%)`	⬇️
lib/options.go	`83.85% <100.00%> (+0.07%)`	⬆️
lib/executor/vu_handle.go	`94.39% <0.00%> (-0.94%)`	⬇️
js/runner.go	`81.86% <0.00%> (-0.54%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa1fd6a...951fcba. Read the comment docs.

na--

Haven't reviewed the code yet, but you seem to have committed a whole k6 binary in one of the commits, so please clean this up and force-push: https://github.com/k6io/k6/blob/aacadb7e7a69a7b2b828df3ac787d018641c227b/js/compiler/k6

Noticed this when I git pulled the latest changes and it took a while:

remote: Total 116 (delta 78), reused 110 (delta 75), pack-reused 0
Receiving objects: 100% (116/116), 15.37 MiB | 1.41 MiB/s, done.

😂

mstoykov · 2021-08-20T13:43:12Z

This is now ready for review 🎉 Although definitely headed for 0.35.0 not 0.34.0

It seems to finally be working very well for everything that I have tried and doesn't seem to have all that big of performance implications - on a script taking 2m20s and 1.7GB it was taking more or less the same time (it fluctuates even without it) and 100mb more (again it fluctuates this is the biggest I have seen).

Notes:

in case we need to be able to disable and reenable it I think (by idea from @na-- ) that we can just use the compatibilityMode possibly with 2 more values to either disable or enable the sourcemaps depending on how it turns out
it used to have an additional flag which is why some of the additional changes were made. The whole compilerOptions can be removed but maybe it's time to make something like that given how many arguments it's taking now :(
obviously, we will have to wait for the source map fixes to be merged

mstoykov · 2021-08-20T14:18:54Z

of course I have now found that one of my even bigger scripts (24k from the cloud request builder) which takes 1min and 1.7gb(the other script was only 10k but with some posts) now takes 20 seconds more and even more importantly 5.5gb of memory o.O I have no idea why this is happening, but I guess we need to look into it some more :(

mstoykov · 2021-10-25T07:51:17Z

js/bundle.go

+	c.COpts = compiler.CompilerOptions{
+		CompatibilityMode: compatMode,
+		Strict:            true,
+		SourceMapEnabled:  true,
+		SourceMapLoader:   generateSourceMapLoader(logger, filesystems),
+	}


This can probably be reverted, but still seems kind of nicer to me so I am leaving it for now

mstoykov · 2021-11-10T14:17:08Z

js/bundle.go

+	c.COpts = compiler.Options{
+		Strict:            true,
+		CompatibilityMode: compatMode,
+		SourceMapEnabled:  true,
+		SourceMapLoader:   generateSourceMapLoader(logger, arc.Filesystems),
+	}
+	pgm, _, err := c.Compile(string(arc.Data), arc.FilenameURL.String(), true, c.COpts)


This can probably be reverted, but still seems kind of nicer to me so I am leaving it for now

Do you mean removing the "", ""? If yes then I agree, it's more explicit.

yeah, I mean that currently, this doesn't let you wrap the code in w/e you want. If it's dependency it will wrap it in the function(module, exports ... wrapper if not it won't. And all of that with 1 boolean instead of 2 strings.

This was never used for anything apart from that outside of tests so 🤷 it seems like a good change to me

codebien

Thanks for working on it 👏, I left a few comments just to "start the discussion". I expect I need to play a bit more with the generated Source Maps.

I expect this will be an important feature, it would be appreciated if we could maintain the commit history atomic and with detailed commit messages I think it could help future readers.

now takes 20 seconds more and even more importantly 5.5gb of memory

I expect all the JSON marshal/unmarshal ops could have an important impact with big source maps.

js/compiler/compiler.go

codebien · 2021-12-10T16:04:38Z

js/compiler/compiler.go

+type Options struct { // TODO maybe have the fields an exported and use the functional options pattern
+	CompatibilityMode lib.CompatibilityMode
+	SourceMapEnabled  bool
+	// TODO maybe move only this in the compiler itself and leave ht rest as parameters to the Compile


Is it expected to do it before merge?

I guess it depends of we want to do it or not. I kind of prefer the options struct as it seems more ... structured ( 😉 😉 ). If we want to keep it I will drop the TODO, otherwise I will need to DO it.

👍 for options struct
Also, it seems Compile is called only in 3 places, in bundle.js and in initcontext.js: it doesn't look as if anything would need an overwrite of compiler options, etc. So I think it can stay simple. Unless there is an expectation of that changing..

👍 same for me for keeping it and removing the comment

js/compiler/compiler.go

codebien · 2021-12-10T16:35:48Z

js/bundle.go

+	c.COpts = compiler.Options{
+		Strict:            true,
+		CompatibilityMode: compatMode,
+		SourceMapEnabled:  true,
+		SourceMapLoader:   generateSourceMapLoader(logger, arc.Filesystems),
+	}
+	pgm, _, err := c.Compile(string(arc.Data), arc.FilenameURL.String(), true, c.COpts)


Do you mean removing the "", ""? If yes then I agree, it's more explicit.

codebien · 2021-12-10T16:42:46Z

js/compiler/compiler.go

@@ -86,10 +87,13 @@ var (
 	globalBabel        *babel        // nolint:gochecknoglobals
 )

+const sourceMapURLFromBabel = "k6://internal-should-not-leak/file.map"


What does it mean? 🤔 Or better, I think that I know it but a comment could help.

This is literally a constant that babel sets in the file which tells goja (in this case) where the sourcemap is. Goja asks k6 to get this and we return the sourcemap babel generated. It's just a particular very unlikely value for a real sourcemap URL ;)

Partially out of curiosity: what is the worst that can happen if someone does use this internal URL deliberately for external sourcemap?

The code returning the sourcemap to goja matches first on it and returns an internal field that get's set when babel transforms. If babel hasn't ran the field is nil - so nothing, if it has it would've set this url.

js/compiler/compiler.go

js/compiler/compiler_test.go

yorugac

If I understood correctly, this PR has some pending design decisions yet so I just added some comments and a question, please see.

ATM, it seems rather straight-forward in terms of implementation changes even with my lacking knowledge of sourcemaps spec 😅

And one more thing: TestSourceMapsExternal shows one example of externally generated sourcemap but it seems like there's more than one external tool that can do that. Are there any expectations of the range of those tools working: are all of them going to work or only some of them, etc. ?

yorugac · 2021-12-10T16:38:42Z

js/compiler/compiler.go

+
+	// ';' is the separator between lines so just adding 1 will make all mappings be for the line after which they were
+	// originally
+	m["mappings"] = ";" + m["mappings"].(string)


Can there be a case when m doesn't contain "mappings" field here? I assume this field is part of the sourcemap format and should be present but I wonder about "broken input" cases, etc.

It seems extremely unlikely as that is the one thing that sourcemaps are about ;). But it can't hurt checking for it I guess.

yorugac · 2021-12-13T09:11:47Z

js/compiler/compiler.go

+type Options struct { // TODO maybe have the fields an exported and use the functional options pattern
+	CompatibilityMode lib.CompatibilityMode
+	SourceMapEnabled  bool
+	// TODO maybe move only this in the compiler itself and leave ht rest as parameters to the Compile


👍 for options struct
Also, it seems Compile is called only in 3 places, in bundle.js and in initcontext.js: it doesn't look as if anything would need an overwrite of compiler options, etc. So I think it can stay simple. Unless there is an expectation of that changing..

js/compiler/compiler.go

codebien

Few additional comments

codebien · 2021-12-13T09:36:52Z

js/compiler/compiler.go

 // A Compiler compiles JavaScript source code (ES5.1 or ES6) into a goja.Program
 type Compiler struct {
 	logger logrus.FieldLogger
 	babel  *babel
+	COpts  Options // TODO change this, this is just way faster


Can you give a bit more context for this TODO comment? Like what is your suggestion?

go.mod

js/bundle.go

codebien · 2021-12-13T14:05:05Z

js/compiler/compiler.go

+type Options struct { // TODO maybe have the fields an exported and use the functional options pattern
+	CompatibilityMode lib.CompatibilityMode
+	SourceMapEnabled  bool
+	// TODO maybe move only this in the compiler itself and leave ht rest as parameters to the Compile


👍 same for me for keeping it and removing the comment

codebien

Seems we have done here! 👏

olegbespalov · 2022-01-12T10:15:36Z

js/compiler/compiler.go

+// compilationState is helper struct to keep the state of a compilation
+type compilationState struct {
+	// set when we couldn't load external source map so we can try parsing without loading it
+	couldntLoadSourceMap bool


I'm wondering if we can avoid this property. For example, using the wrapped errors and checks like errors.Is. It seems simpler and more go idiomatic, wdyt?

The problem @olegbespalov is that the error doesn't get propaged through goja and fixing that will take quite a lot refactoring IIRC, so I opted for not trying to do that first.

😢 I see, thanks for clarifying 👍

js/compiler/compiler.go

na--

LGTM. Things are a bit convoluted, but they should be simplified when we get rid of Babel 🤞

na--

🤞

yorugac

It seems to be more readable than before 👍 though it might be just multiple re-reads effect 🙂 I have some remarks on comments mostly, please see.

yorugac · 2022-01-12T10:48:20Z

js/compiler/compiler.go

+type compilationState struct {
+	// set when we couldn't load external source map so we can try parsing without loading it
+	couldntLoadSourceMap bool
+	// srcMap is the current full sourceMap that has been generated read so far


Is it 'generated' or 'read' here?

it's an or - it's either read from disk or generated by babel

js/compiler/compiler.go

yorugac · 2022-01-12T12:00:40Z

js/compiler/compiler.go

@@ -86,10 +87,13 @@ var (
 	globalBabel        *babel        // nolint:gochecknoglobals
 )

+const sourceMapURLFromBabel = "k6://internal-should-not-leak/file.map"


Partially out of curiosity: what is the worst that can happen if someone does use this internal URL deliberately for external sourcemap?

mstoykov · 2022-01-12T12:17:24Z

A problem I found around doing some final testing and especially after #2082 (comment):

If the sourcemap is actually broken (in some obvious way) or just can't be parsed as JSON in general this will now abort the test and at least in some cases we will have hard time noticing it as you can have:

var o={d:(e,r)=>{for(var t in r)o.o(r,t)&&!o.o(e,t)&&Object.defineProperty(e,t,{enumerable:!0,get:r[t]})},o:(o,e)=>Object.prototype.hasOwnProperty.call(o,e)},e={};o.d(e,{Z:()=>r});const r=()=>{!function(o){throw"cool is cool"}()};var t=e.Z;export{t as default};
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIndlYnBhY2s6Ly8vd2VicGFjay9ib290c3RyYXAiLCJ3ZWJwYWNrOi8vL3dlYnBhY2svcnVudGltZS9kZWZpbmUgcHJvcGVydHkgZ2V0dGVycyIsIndlYnBhY2s6Ly8vd2VicGFjay9ydW50aW1lL2hhc093blByb3BlcnR5IHNob3J0aGFuZCIsIndlYnBhY2s6Ly8vLi90ZXN0MS50cyJdLCJuYW1lcyI6WyJfX3dlYnBhY2tfcmVxdWlyZV9fIiwiZXhwb3J0cyIsImRlZmluaXRpb24iLCJrZXkiLCJvIiwiT2JqZWN0IiwiZGVmaW5lUHJvcGVydHkiLCJlbnVtZXJhYmxlIiwiZ2V0Iiwib2JqIiwicHJvcCIsInByb3RvdHlwZSIsImhhc093blByb3BlcnR5IiwiY2FsbCIsInMiLCJjb29sVGhyb3ciXSwibWFwcGluZ3MiOiJBQUNBLElBQUlBLEVBQXNCLENDQTFCLEVBQXdCLENBQUNDLEVBQVNDLEtBQ2pDLElBQUksSUFBSUMsS0FBT0QsRUFDWEYsRUFBb0JJLEVBQUVGLEVBQVlDLEtBQVNILEVBQW9CSSxFQUFFSCxFQUFTRSxJQUM1RUUsT0FBT0MsZUFBZUwsRUFBU0UsRUFBSyxDQUFFSSxZQUFZLEVBQU1DLElBQUtOLEVBQVdDLE1DSjNFLEVBQXdCLENBQUNNLEVBQUtDLElBQVVMLE9BQU9NLFVBQVVDLGVBQWVDLEtBQUtKLEVBQUtDLEksc0JDR2xGLGNBSEEsU0FBbUJJLEdBQ2YsS0FBTSxlQUdOQyxJIiwiZmlsZSI6InRlc3QxLmpzIiwic291cmNlc0NvbnRlbnQiOlsiLy8gVGhlIHJlcXVpcmUgc2NvcGVcbnZhciBfX3dlYnBhY2tfcmVxdWlyZV9fID0ge307XG5cbiIsIi8vIGRlZmluZSBnZXR0ZXIgZnVuY3Rpb25zIGZvciBoYXJtb255IGV4cG9ydHNcbl9fd2VicGFja19yZXF1aXJlX18uZCA9IChleHBvcnRzLCBkZWZpbml0aW9uKSA9PiB7XG5cdGZvcih2YXIga2V5IGluIGRlZmluaXRpb24pIHtcblx0XHRpZihfX3dlYnBhY2tfcmVxdWlyZV9fLm8oZGVmaW5pdGlvbiwga2V5KSAmJiAhX193ZWJwYWNrX3JlcXVpcmVfXy5vKGV4cG9ydHMsIGtleSkpIHtcblx0XHRcdE9iamVjdC5kZWZpbmVQcm9wZXJ0eShleHBvcnRzLCBrZXksIHsgZW51bWVyYWJsZTogdHJ1ZSwgZ2V0OiBkZWZpbml0aW9uW2tleV0gfSk7XG5cdFx0fVxuXHR9XG59OyIsIl9fd2VicGFja19yZXF1aXJlX18ubyA9IChvYmosIHByb3ApID0+IChPYmplY3QucHJvdG90eXBlLmhhc093blByb3BlcnR5LmNhbGwob2JqLCBwcm9wKSkiLCJmdW5jdGlvbiBjb29sVGhyb3coczogc3RyaW5nKSB7XG4gICAgdGhyb3cgXCJjb29sIFwiKyBzXG59XG5leHBvcnQgZGVmYXVsdCAoKSA9PiB7XG4gICAgY29vbFRocm93KFwiaXMgY29vbFwiKVxufTtcbiJdLCJzb3VyY2VSb290IjoiIn0=

A file with embeded sourcemap, if you now corrupt this (delete some symbols in the base64 encoded string) The error we get back is something like(using spew-go):

(*goja.Exception)(0xc001e4b920)(SyntaxError: file:///home/mstoykov/work/k6io/k6/test1.js: invalid character '9' after object key at <internal/k6/compiler/lib/babel.min.js>:2:28536(109))

In this particular case as this code still goes through babel and this is what it returns. if it didn't we will get

(parser.ErrorList) (len=1 cap=1) file:///home/mstoykov/work/k6io/k6/test1.js: Line 3:1 Could not load source map: illegal base64 data at input byte 1724

Which at least is somewhat matchable with a string 🤷 .

I don't think this is a particularly likely occurrence and will become less of a problem as time goes on and hopefully we use less babel.

We can also always on error with sourcemap enabled retry with it disabled, but this goes and more in trying to handle random corner cases that I doubt will be hit by anyone. And obviously we can always revert this or add the necessary changes or add a flag to enable/disable source map if the need arises.

This includes both support for any sourcemap found through the source file and generating sourcemaps when going through babel. There are additional fixes for trying to fix off by 1 line errors in imports, but those may need further work. On not being able to load a sourcemap a warning is emitted but the file is still parsed and compiled just without sourcemaps fixes #1789, #1804

this hopefully has some better performance but more importantly the generated code is more readable without it, while the lines don't match.

mstoykov requested review from na--, imiric and codebien July 2, 2021 15:02

na-- requested changes Jul 9, 2021

View reviewed changes

mstoykov force-pushed the feature/SourceMaps branch from 3cc4ad2 to 951fcba Compare July 12, 2021 10:49

mstoykov force-pushed the feature/SourceMaps branch from 951fcba to 683d3c1 Compare August 18, 2021 10:16

mstoykov mentioned this pull request Aug 20, 2021

JS script stack traces not referencing the correct lines of code in imported modules #1804

Closed

mstoykov requested a review from na-- August 20, 2021 13:50

na-- added this to the v0.35.0 milestone Aug 25, 2021

mstoykov force-pushed the feature/SourceMaps branch from 1065bcf to 218d8f4 Compare October 25, 2021 07:49

mstoykov changed the base branch from master to updateGoja October 25, 2021 07:49

mstoykov commented Oct 25, 2021

View reviewed changes

mstoykov force-pushed the updateGoja branch from c2b82fd to bd78f63 Compare October 26, 2021 07:54

Base automatically changed from updateGoja to master October 26, 2021 11:45

na-- modified the milestones: v0.35.0, v0.36.0 Oct 27, 2021

mstoykov mentioned this pull request Nov 3, 2021

Drop map structure #2223

Merged

na-- changed the title ~~Feature/source maps~~ Source map support Nov 10, 2021

mstoykov force-pushed the feature/SourceMaps branch from efade35 to e9edc51 Compare November 10, 2021 14:16

mstoykov commented Nov 10, 2021

View reviewed changes

mstoykov mentioned this pull request Nov 19, 2021

Better detection that a value should be pushed go-sourcemap/sourcemap#16

Merged

mstoykov force-pushed the feature/SourceMaps branch from e905f38 to a8e5ec1 Compare November 19, 2021 14:05

mstoykov requested a review from a team December 2, 2021 11:36

codebien requested changes Dec 10, 2021

View reviewed changes

yorugac reviewed Dec 13, 2021

View reviewed changes

codebien requested changes Dec 13, 2021

View reviewed changes

mstoykov force-pushed the feature/SourceMaps branch from 032c028 to db288cc Compare January 5, 2022 17:14

codebien previously approved these changes Jan 5, 2022

View reviewed changes

olegbespalov reviewed Jan 12, 2022

View reviewed changes

na-- reviewed Jan 12, 2022

View reviewed changes

js/compiler/compiler.go Outdated Show resolved Hide resolved

na-- reviewed Jan 12, 2022

View reviewed changes

js/compiler/compiler.go Show resolved Hide resolved

na-- reviewed Jan 12, 2022

View reviewed changes

js/compiler/compiler.go Show resolved Hide resolved

mstoykov dismissed codebien’s stale review via 44c2849 January 12, 2022 10:43

codebien requested changes Jan 12, 2022

View reviewed changes

js/compiler/compiler.go Outdated Show resolved Hide resolved

na-- previously approved these changes Jan 12, 2022

View reviewed changes

mstoykov dismissed na--’s stale review via 4a044e2 January 12, 2022 10:52

codebien previously approved these changes Jan 12, 2022

View reviewed changes

mstoykov dismissed codebien’s stale review via b8455cc January 12, 2022 11:13

mstoykov force-pushed the feature/SourceMaps branch 2 times, most recently from 15a6fac to 5e705b3 Compare January 12, 2022 11:47

na-- previously approved these changes Jan 12, 2022

View reviewed changes

yorugac previously approved these changes Jan 12, 2022

View reviewed changes

mstoykov dismissed stale reviews from yorugac and na-- via 51b7df6 January 12, 2022 12:39

na-- previously approved these changes Jan 12, 2022

View reviewed changes

mstoykov added 4 commits January 12, 2022 15:02

Update sourcemap dependancy to get fixes

e66f2dc

Disable retainLines on babel when using sourcemaps

b51774e

this hopefully has some better performance but more importantly the generated code is more readable without it, while the lines don't match.

Add test with inline source map

9ae6aaa

mstoykov dismissed na--’s stale review via 9ae6aaa January 12, 2022 13:07

mstoykov force-pushed the feature/SourceMaps branch from 51b7df6 to 9ae6aaa Compare January 12, 2022 13:07

na-- approved these changes Jan 12, 2022

View reviewed changes

codebien approved these changes Jan 12, 2022

View reviewed changes

mstoykov merged commit 6783596 into master Jan 12, 2022

mstoykov deleted the feature/SourceMaps branch January 12, 2022 13:17

Source map support #2082

Source map support #2082

Conversation

mstoykov commented Jul 2, 2021

mstoykov commented Jul 2, 2021

codecov-commenter commented Jul 2, 2021 • edited Loading

Codecov Report

na-- left a comment

Choose a reason for hiding this comment

mstoykov commented Aug 20, 2021

mstoykov commented Aug 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorugac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

yorugac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov commented Jan 12, 2022

codecov-commenter commented Jul 2, 2021 •

edited

Loading

codebien left a comment •

edited

Loading

codebien Dec 10, 2021 •

edited

Loading