Closed Bug 1291877 Opened 3 years ago Closed 3 years ago

Source map optimization in console due to Unique Locations

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: jsantell, Assigned: jbhoosreddy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [source-maps])

Attachments

(1 file, 2 obsolete files)

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.
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).
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.
Assignee: nobody → jaideepb
Status: NEW → ASSIGNED
Attached patch 1291877.patch [WIP] (obsolete) — Splinter Review
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.
Attachment #8777558 - Flags: feedback?(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
Attachment #8777558 - Flags: feedback?(jsantell)
Attached patch 1291877.patch (obsolete) — Splinter Review
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.
Attachment #8777558 - Attachment is obsolete: true
Attachment #8779605 - Flags: review?(jsantell)
Whiteboard: [source-maps]
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
Attachment #8779605 - Flags: review?(jsantell) → review+
Attachment #8779605 - Attachment is obsolete: true
Attachment #8781858 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/87d16ac23161
Source map optimization in console due to Unique Locations. r=jsantell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/87d16ac23161
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.