This looks great, I thought it would be much more work to convey all original location.
I do have a few high level questions before moving forward.

About DAMP I saw that the pretty print test is against main.js which is significantly smaller than minified.js:
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/tests/debugger/custom.js#207
It might be nice to go with the largest file. It may help catch more subtle regression/improvements.

minified isn't loaded by default, but we could add a sub test over there, after having opened the large minified file:
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/tests/debugger/custom.js#207

Otherwise, what is the current conclusion of DAMP about this patch queue?

I could see two ways to optimize this even further:

• still buffer some writes, for example all spaces + comments, which we add *a lot* while pretty printing. You went in that direction with your work in addComment.
• pass null line, column to SourceNode for spaces + comments as this isn't useful to map them. It isn't clear if the previous code was trying to do that with -1. SourceMap doesn't seem to handle -1 any differently than a valid line :/ The SourceMap lib ignore SourceNode with null values and doesn't add them to the mapping: https://searchfox.org/mozilla-central/rev/9dfda5ccb0fc42d7666a54b1caf1af6525e49694/devtools/client/shared/vendor/source-map/lib/source-node.js#356-360 But I'm not 100% sure it doesn't mess up with source map computation...

In D168383#5546396, @ochameau wrote:

About DAMP I saw that the pretty print test is against main.js which is significantly smaller than minified.js:
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/tests/debugger/custom.js#207
It might be nice to go with the largest file. It may help catch more subtle regression/improvements.

I tried using minified.js when I added the DAMP test but it was super slow (5s on my machine)
Pretty printing main.js still takes around 500ms on TRY, and I feel like it's enough?

Otherwise, what is the current conclusion of DAMP about this patch queue?

Depending on the platform, it's either very little improvement to 5% improvement, adding the mappings for columns + passing the sourceMap directly instead of the mappings

I could see two ways to optimize this even further:

• still buffer some writes, for example all spaces + comments, which we add *a lot* while pretty printing. You went in that direction with your work in addComment.
• pass null line, column to SourceNode for spaces + comments as this isn't useful to map them. It isn't clear if the previous code was trying to do that with -1. SourceMap doesn't seem to handle -1 any differently than a valid line :/ The SourceMap lib ignore SourceNode with null values and doesn't add them to the mapping: https://searchfox.org/mozilla-central/rev/9dfda5ccb0fc42d7666a54b1caf1af6525e49694/devtools/client/shared/vendor/source-map/lib/source-node.js#356-360 But I'm not 100% sure it doesn't mess up with source map computation...

I can try that, see if it doesn't break anything and if that does bring more improvements

In D168383#5546396, @ochameau wrote:

I could see two ways to optimize this even further:

• […]
• pass null line, column to SourceNode for spaces + comments as this isn't useful to map them. It isn't clear if the previous code was trying to do that with -1. SourceMap doesn't seem to handle -1 any differently than a valid line :/ The SourceMap lib ignore SourceNode with null values and doesn't add them to the mapping: https://searchfox.org/mozilla-central/rev/9dfda5ccb0fc42d7666a54b1caf1af6525e49694/devtools/client/shared/vendor/source-map/lib/source-node.js#356-360 But I'm not 100% sure it doesn't mess up with source map computation...

So I tried passing null line/column for the comments , and this breaks
SourceNode only ignores those if they're before any other valid chunks: https://searchfox.org/mozilla-central/rev/9dfda5ccb0fc42d7666a54b1caf1af6525e49694/devtools/client/shared/vendor/source-map/lib/source-node.js#356-394

var sourceMappingActive = false;
// […]
if (
  original.source !== null &&
  original.line !== null &&
  original.column !== null
) {
  // […]
  sourceMappingActive = true;
} else if (sourceMappingActive) {
  map.addMapping({
    generated: {
      line: generated.line,
      column: generated.column,
    },
  });
}

In D168383#5548180, @nchevobbe wrote:

In D168383#5546396, @ochameau wrote:

About DAMP I saw that the pretty print test is against main.js which is significantly smaller than minified.js:
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/tests/debugger/custom.js#207
It might be nice to go with the largest file. It may help catch more subtle regression/improvements.

I tried using minified.js when I added the DAMP test but it was super slow (5s on my machine)
Pretty printing main.js still takes around 500ms on TRY, and I feel like it's enough?

Ah ok. I thought that we had better performance and main.js would be significantly faster.
It may make sense to switch to minified.js if we end up speeding up the current implemenation only.
Otherwise 500ms looks like a good baseline for now.

Otherwise, what is the current conclusion of DAMP about this patch queue?

Depending on the platform, it's either very little improvement to 5% improvement, adding the mappings for columns + passing the sourceMap directly instead of the mappings

5% improvement, this is with D168384 ?
It would be nice to know the performance impact of column mapping by itself.
I think we were still having reports about pretty print being too slow for some module.
We might be able to make it fast without column breakpoint, but still as slow as before with column breakpoint.
I'm wondering if that should be an optional feature for large files.

I could see two ways to optimize this even further:

• still buffer some writes, for example all spaces + comments, which we add *a lot* while pretty printing. You went in that direction with your work in addComment.

Any opinion about this?

Also, you mentioned tweaking pretty fast to add more new lines ? I imagine this would also reduce the need of column breakpoint ?

So I tried passing null line/column for the comments , and this breaks
SourceNode only ignores those if they're before any other valid chunks: https://searchfox.org/mozilla-central/rev/9dfda5ccb0fc42d7666a54b1caf1af6525e49694/devtools/client/shared/vendor/source-map/lib/source-node.js#356-394

} else if (sourceMappingActive) {
  map.addMapping({
    generated: {
      line: generated.line,
      column: generated.column,
    },
  });
}

I'm wondering why this codes adds such mapping, which looks useless? It doesn't map original to generated...
https://searchfox.org/mozilla-central/rev/ddbeacddc15008936e8f619c8c3a05fac6eab8d8/devtools/client/shared/vendor/source-map/lib/source-node.js#396-419
But I'm also wondering if you are getting issues because of this other code, auto-adding mappins on newlines which aren't last in the chunk:
https://searchfox.org/mozilla-central/rev/ddbeacddc15008936e8f619c8c3a05fac6eab8d8/devtools/client/shared/vendor/source-map/lib/source-node.js#396-419

May be we should be forking this method for our very own implementation and strip these two pieces that may be irrelevant to us.
The else if (sourceMappingActive) and the for loop.

In D168383#5558606, @ochameau wrote:

Otherwise, what is the current conclusion of DAMP about this patch queue?

Depending on the platform, it's either very little improvement to 5% improvement, adding the mappings for columns + passing the sourceMap directly instead of the mappings

5% improvement, this is with D168384 ?

yes

It would be nice to know the performance impact of column mapping by itself.
I think we were still having reports about pretty print being too slow for some module.
We might be able to make it fast without column breakpoint, but still as slow as before with column breakpoint.
I'm wondering if that should be an optional feature for large files.

D168384: Bug 1815452 - [devtools] Simplify mapping structure posted to/from workers. r=bomsy!,ochameau. alone brings a nice ~37/38% improvement : https://treeherder.mozilla.org/perfherder/compare?originalProject=try&newProject=try&newRevision=120c9b6b7deef5b13589e6e4570b77c495a5973f&framework=12&originalRevision=597d245f8f656ba2cf139d25e401b414418fc46b&page=1
And so yes, we lose a lot of this with columns.
One thing I wonder is if making pretty printing a 2 step process could help in the process:

• first fold would only return the pretty printed text, so we can show something to the user fast
• then we could actually compute the sourceMap and apply it. While waiting for the sourceMap to be applied, we'd grayed-out all the lines in the gutter

I can run some numbers on that to see how faster this would feel for the user, without sacrificing correctness in the end.
How does that sound?

I could see two ways to optimize this even further:

• still buffer some writes, for example all spaces + comments, which we add *a lot* while pretty printing. You went in that direction with your work in addComment.

Any opinion about this?

Yes, we can definitely investigate this

Also, you mentioned tweaking pretty fast to add more new lines ? I imagine this would also reduce the need of column breakpoint ?

Yes, better petty printing would hopefully prevent long line, and thus reduce column breakpoint. But this is still something user would like to see work as in regular script :) (for example the case of column breakpoints in for/while loop condition)

So I tried passing null line/column for the comments , and this breaks
SourceNode only ignores those if they're before any other valid chunks: https://searchfox.org/mozilla-central/rev/9dfda5ccb0fc42d7666a54b1caf1af6525e49694/devtools/client/shared/vendor/source-map/lib/source-node.js#356-394

} else if (sourceMappingActive) {
  map.addMapping({
    generated: {
      line: generated.line,
      column: generated.column,
    },
  });
}

I'm wondering why this codes adds such mapping, which looks useless? It doesn't map original to generated...
https://searchfox.org/mozilla-central/rev/ddbeacddc15008936e8f619c8c3a05fac6eab8d8/devtools/client/shared/vendor/source-map/lib/source-node.js#396-419
But I'm also wondering if you are getting issues because of this other code, auto-adding mappins on newlines which aren't last in the chunk:
https://searchfox.org/mozilla-central/rev/ddbeacddc15008936e8f619c8c3a05fac6eab8d8/devtools/client/shared/vendor/source-map/lib/source-node.js#396-419

May be we should be forking this method for our very own implementation and strip these two pieces that may be irrelevant to us.
The else if (sourceMappingActive) and the for loop.

Yes, it could be nice to see how we could bend SourceNode to work more gracefully with pretty printing.

overall, I'll also do more profiling to see where time is spent, also checking the actual benefit we might get from the newest version of the sourcemap library (i.e. excluding the cost of loading)

I'm landing the optimization patch as part of another bug for now (Bug 1815452)

By not creating SourceNode for each space char, we're making the regression less important (18% vs 25%): https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=2beb53908906ba02303af29fcdc6718758024569&originalSignature=2916453&newSignature=2916453&framework=12&originalRevision=eb42617ad691d224d3c7b1e7c6fa072e62f6ed4f&page=1&showOnlyImportant=1

Code analysis found 1 defect in the diff 683291:

• 1 defect found by test-manifest (Mozlint)

You can run this analysis locally with:

• ./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 683291.

@ochameau about coalescing successive whitespace strings, this is already kind of handled in SourceNode#toStringWithSourceMap:

https://searchfox.org/mozilla-central/rev/416f31a51174620f04fc994d248b664b54517699/devtools/client/shared/vendor/source-map/lib/source-node.js#361-383

if (
  lastOriginalSource !== original.source ||
  lastOriginalLine !== original.line ||
  lastOriginalColumn !== original.column ||
  lastOriginalName !== original.name
) {
  map.addMapping({
    source: original.source,
    original: {
      line: original.line,
      column: original.column,
    },
    generated: {
      line: generated.line,
      column: generated.column,
    },
    name: original.name,
  });
}
lastOriginalSource = original.source;
lastOriginalLine = original.line;
lastOriginalColumn = original.column;
lastOriginalName = original.name;

so here original is a LeafNode in our case; when we walk through all the node, no mapping will be added if the line and column are the same (source is the same for all items, the file url, and name is always null).
Since we get line and column from tokens (i.e. non whitespace), any whitespace children we add after a token gets the same line and column, and so in the end, it won't end up creating a mapping.
To put that in perspective, in the prettyPrint DAMP test:

• we have 88 008 tokens
• the root SourceNode ends up with 145 419 children
• but only 91 337 mappings are added to the SourceMapGenerator

as you can see, the delta between tokens and mappings is not very high.
The only overhead with having a high number of children is looping through all those extra items, but I don't feel like it's a bottleneck

If you're curious, here's a profile with this stack: https://share.firefox.dev/41TTTS1
we spend 25% getting tokens from the script text (using acorn, updating to latest version did not improve the numbers),
~20% in generating the source map, half writing the dql, and half sorting the mappings. We could look into how we could _not_ sort, but since we need to swap original and generated locations, that's a bit tricky
~10% in toStringWithSourceMap, where we iterate through children and call addMapping; we create a lot of new objects which we then translate back into something similar to the original shape of LeafNode/SourceNode (see https://searchfox.org/mozilla-central/rev/416f31a51174620f04fc994d248b664b54517699/devtools/client/shared/vendor/source-map/lib/source-map-generator.js#124-131). So maybe we could simplify things in the source map library a bit

then we have some items under ~10% that I tried to make faster (e.g. prependWhitespace), without much success.

ah, but it looks like it doesn't fix the original issue and the added test now fails :/

Do you have explanation on why it fails?
I'm wondering if that's because of this:
https://searchfox.org/mozilla-central/rev/cd2121e7d83af1b421c95e8c923db70e692dab5f/devtools/client/shared/vendor/source-map/lib/source-node.js#395-396
The existing buffering code still pass the complete source content to SourceMap lib,
in your WIP you only pass partial content to it, so this code forcing the adding of mapping on newlines if bypassed.

In D168383#5659855, @nchevobbe wrote:

@ochameau about coalescing successive whitespace strings, this is already kind of handled in SourceNode#toStringWithSourceMap:

https://searchfox.org/mozilla-central/rev/416f31a51174620f04fc994d248b664b54517699/devtools/client/shared/vendor/source-map/lib/source-node.js#361-383

if (
  lastOriginalSource !== original.source ||
  lastOriginalLine !== original.line ||
  lastOriginalColumn !== original.column ||
  lastOriginalName !== original.name
) {
  map.addMapping({
    source: original.source,
    original: {
      line: original.line,
      column: original.column,
    },
    generated: {
      line: generated.line,
      column: generated.column,
    },
    name: original.name,
  });
}
lastOriginalSource = original.source;
lastOriginalLine = original.line;
lastOriginalColumn = original.column;
lastOriginalName = original.name;

so here original is a LeafNode in our case; when we walk through all the node, no mapping will be added if the line and column are the same (source is the same for all items, the file url, and name is always null).
Since we get line and column from tokens (i.e. non whitespace), any whitespace children we add after a token gets the same line and column, and so in the end,

This isn't clear to me why spaces and comments would all end up on the same line and column?
I can see that prependWhiteSpace flags all added code on the same line/column, but we have spaces added elsewhere, like here:
https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/workers/pretty-print/pretty-fast.js#891
https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/workers/pretty-print/pretty-fast.js#933
Or may be you only meant spaces related to prependWhiteSpace?

it won't end up creating a mapping.
To put that in perspective, in the prettyPrint DAMP test:

• we have 88 008 tokens
• the root SourceNode ends up with 145 419 children
• but only 91 337 mappings are added to the SourceMapGenerator

as you can see, the delta between tokens and mappings is not very high.
The only overhead with having a high number of children is looping through all those extra items, but I don't feel like it's a bottleneck

If you're curious, here's a profile with this stack: https://share.firefox.dev/41TTTS1
we spend 25% getting tokens from the script text (using acorn, updating to latest version did not improve the numbers),
~20% in generating the source map, half writing the dql, and half sorting the mappings. We could look into how we could _not_ sort, but since we need to swap original and generated locations, that's a bit tricky
~10% in toStringWithSourceMap, where we iterate through children and call addMapping; we create a lot of new objects which we then translate back into something similar to the original shape of LeafNode/SourceNode (see https://searchfox.org/mozilla-central/rev/416f31a51174620f04fc994d248b664b54517699/devtools/client/shared/vendor/source-map/lib/source-map-generator.js#124-131). So maybe we could simplify things in the source map library a bit

Yes, that's a very good analysis. There is a lot of GCs in the profile. It is probably related to this.
The transient objects created by SourceNode.toStringWithSourceMap as argument to SourceMapGenerated.addMapping
are probably adding lots of GC overhead.
Lowering the number of SourceNode won't have an impact on this as you said, they are filtered out by toStringWithSourceMap.
Reducer the number of SourceNode would only have an impact on the big GC pause which happens later. This is not visible on your profile as you probably focused only on JS code. But it is clearly visible on DAMP.

So if there is easy ways to prevent spawning SourceNodes, we should do it, even if that doesn't show up in DAMP.
Note that we call write without line/column in many places
Otherwise, I agree, a good way to improve this further would be to contribute to Source-map library in order to avoid these transient objects.
But it sounds like it would require migrating to latest revision and get back to the issue of whatwg-url :(

then we have some items under ~10% that I tried to make faster (e.g. prependWhitespace), without much success.

What's the current numbers reported by DAMP?
If the numbers are reasonable enough, the patch is looking great to me as-is.
We could probably followup on improving pretty printed.

In D168383#5663152, @ochameau wrote:

ah, but it looks like it doesn't fix the original issue and the added test now fails :/

Do you have explanation on why it fails?

This was a temporary hiccup, it runs fine now

This isn't clear to me why spaces and comments would all end up on the same line and column?

when parsing the function script, the token list that we get don't have any "whitespace tokens", and in this patch, whenever we call write, we pass a line and a column information, that we do get from the previous token we already handled and for which we already called write on

so if we handled a token ( , at (1,2), the space we'll add after that we'll also refer to (1,2), and if we add a linebreak after, it will still refer to (1,2). So here we'd end up with 3 SourceNode, the first one, for (, would translate into a mapping, the but then the ones for the space and the linebreak won't, because the location was already mapped.
Does that make snse?

Yes, that's a very good analysis. There is a lot of GCs in the profile. It is probably related to this.
The transient objects created by SourceNode.toStringWithSourceMap as argument to SourceMapGenerated.addMapping
are probably adding lots of GC overhead.
Lowering the number of SourceNode won't have an impact on this as you said, they are filtered out by toStringWithSourceMap.
Reducer the number of SourceNode would only have an impact on the big GC pause which happens later. This is not visible on your profile as you probably focused only on JS code. But it is clearly visible on DAMP.
So if there is easy ways to prevent spawning SourceNodes, we should do it, even if that doesn't show up in DAMP.

In D171477 , we're not create SourceNode anymore, but simpler class instance. I tried to replace the LeafSourceNode by simple objects I was creating in write, but this performed worse.

Note that we call write without line/column in many places

I don't think this is correct? I went through all write callsites in this patch and made sure to always pass line and columns (otherwise I think toStringWithSourceMap or toJson was throwing)

Otherwise, I agree, a good way to improve this further would be to contribute to Source-map library in order to avoid these transient objects.
But it sounds like it would require migrating to latest revision and get back to the issue of whatwg-url :(

So I upgraded to latest published version of the SourceMap library in D171476 and didn't saw the performance regression I observed earlier. I think before I was trying to require the file we have in mc, whereas now, I'm just using the one from npm which is put in the bundle.

then we have some items under ~10% that I tried to make faster (e.g. prependWhitespace), without much success.

What's the current numbers reported by DAMP?
If the numbers are reasonable enough, the patch is looking great to me as-is.
We could probably followup on improving pretty printed.

Last time I pushed I was seeing between 25 et 35% regression (https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=e24ef90732ecbd4a90ec9596c1069dd3435a0ef6&originalSignature=2916453&newSignature=2916453&framework=12&application=&originalRevision=36d8348be5bc562c61d815c7299caf3cd18c7d06&page=1&showOnlyImportant=1) which is still lower than what we started with (https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&highlightedRevisions=36d8348be5bc&highlightedRevisions=e24ef90732ec&series=try,4592498,1,12&timerange=5184000 , on this platform, we went from ~600ms to ~320ms, and with this patch, this goes up to 420ms, which is still quite faster than the original numbers)

Code analysis found 1 defect in the diff 691099:

• 1 defect found by test-manifest (Mozlint)

1 issue closed compared to the previous diff 688576.

You can run this analysis locally with:

• ./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 691099.

Thanks a lot for the suggestion Alex, this is simpler to follow, and a local profile seems to indicate some nice performance, let's see if damp is happy with it too: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=063d45fea42612eedc10559d798ada0ffbf2a398&newProject=try&newRevision=2956e9c3aa4f163c1d476503ace1b3043b268fbd&page=1&framework=12

Code analysis found 1 defect in the diff 693043:

• 1 defect found by test-manifest (Mozlint)

You can run this analysis locally with:

• ./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 693043.