1291877 - Source map optimization in console due to Unique Locations

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Right now, we cache multiple location calls over RDP, but if we have multiple unique locations for a non-source mapped file, we make redundant calls to the server.

We can implement a map of URL to SOURCE_MAP_STATUS, either true, false or undefined, defaulting all logged locations to undefined. Upon location resolve, if undefined or true for that URL, we continue attempt to resolve. If we get back the same location (is this possible with a valid source map? Or maybe we can also return the source meta with the `isSourceMapped` property?), then we can tag the URL's SOURCE_MAP_STATUS as `false`, and all other attempts to resolve locations with this URL are just a resolve-noop.

On 'source-updated', if a valid event (passing the conditionals), we can then tag or retag the SOURCE_MAP_STATUS for that URL are `true` if the source isSourceMapped, and within `onSourceUpdated`, we resolve all current locations anyway to reattempt, which would resolve to the new source mapped version.

This is a potential plan Jaideep and I discussed, but wondering if it's worth the effort with client side source mapping on the horizon.

I can explain more clearly if needed, I'm sure.

Comment 1

[James Long (:jlongster)]

It's probably worth doing unless it introduces a lot of complexity. Client-side sourcemaps realistically won't be ready and preffed on for a few months (maybe earlier, but want to be pessimistic).

Comment 2

[James Long (:jlongster)]

Actually, there's a much easier solution now (I think). Sources now provide a `sourceMapURL` property on the source form, so the SourceClient knows if it has a sourcemap or not. Don't you have access to the SourceClient of a source? If so, you already know that it's not sourcemapped if the `sourceMapURL` is null.

Comment 3

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1291877.patch [WIP]

Based on James' comments and looking into the existing implementation.

I have something like this in mind. But maybe we can make it more robust, in terms of how those booleans are set for each URL.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8777558 [details] [diff] [review]
1291877.patch [WIP]

Review of attachment 8777558 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/framework/source-map-service.js
@@ +18,5 @@
>  
>  function SourceMapService(target) {
>    this._target = target;
>    this._locationStore = new LocationStore();
> +  this._shouldSourceMap = new Map();

This phrasing implies we're in control of the source maps here -- we should call this `isSourceMapped` or something

@@ +60,5 @@
>   */
>  SourceMapService.prototype.subscribe = function (location, callback) {
>    this.on(serialize(location), callback);
>    this._locationStore.set(location);
> +  if (location.url && this._shouldSourceMap.get(location.url) === false) {

if `location.url` does not exist, we should bail out immediately and probably throw

@@ +136,5 @@
>    // we can have no actionable updates as this is just a new normal source.
>    // Also abort if there's no `url`, which means it's unsourcemappable anyway,
>    // like an eval script.
>    if (!source.url || type === "newSource" && !source.isSourceMapped) {
> +    if (source.url) {

These nested conditionals are a bit confusing, and should have more comments explaining it -- also possibly use James' method mentioned

Comment 5

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1291877.patch

In addition to adding the small optimization, updated the comments to better reflect how source-mapping is handled by the source map service. also, fixed linting issues. I'm fairly confident/hopeful that this should handle most possible scenarios.
Let me know what you think.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8779605 [details] [diff] [review]
1291877.patch

Review of attachment 8779605 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Jaideep, do you have time to go through landing this?

::: devtools/client/framework/source-map-service.js
@@ +145,5 @@
> +  // Check Source Actor for sourceMapURL property (after Firefox 48)
> +  // If not present, utilize isSourceMapped and isPrettyPrinted properties
> +  // to estimate if a source is not source-mapped.
> +  const isNotSourceMapped = !(source.sourceMapURL ||
> +  (source.isSourceMapped || source.isPrettyPrinted));

nit: indentation on second line, and shouldn't need nested parens here since they're all logical boolean operators

Comment 7

[Jaideep Bhoosreddy [:jbhoosreddy]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4fa98dfc80b

Comment 8

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1291877.patch [1.0]

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/87d16ac23161
Source map optimization in console due to Unique Locations. r=jsantell

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87d16ac23161