Closed Bug 975750 Opened 6 years ago Closed 6 years ago

HttpDataUsage should not do main thread I/O

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: Gavin, Assigned: mcmanus)

References

(Blocks 1 open bug)

Details

(Keywords: main-thread-io, perf)

Attachments

(2 files)

Per http://ravitillo.wordpress.com/2014/02/21/main-thread-io-hall-of-shame/, httpDataUsage.dat seems to be a top main-thread-IO offender.
Attached patch patchSplinter Review
This switches the I/O to OS.File, and reworks things a little bit:
- gets rid of _locked, since OS.File handles sequencing the operations on its own
- consolidate the "read file/update telemetry/write file" task into the "updateStats" function, and have it return a promise that can be used to ensure that the operation completes at shutdown (via AsyncShutdown)
- track whether updateStats' task is pending via _updatePending, just to avoid the very unlikely scenario of quit-application being fired in the middle of one of our idle updates

This component doesn't seem to have tests, but I think I tested all of the change code paths manually (either by calling observe() directly from the Browser Console, or editing the contents of httpDataUsage.dat manually).
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #8380244 - Flags: review?(mcmanus)
Attachment #8380244 - Flags: review?(dteller)
Blocks: 572459
No longer blocks: 649889
No longer blocks: 975980
Comment on attachment 8380244 [details] [diff] [review]
patch

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

This should definitely be nicer on the main thread I/O.

::: netwerk/protocol/http/HttpDataUsage.js
@@ +81,5 @@
>          }
>          this._isIdleObserver = false;
>      },
>  
> +    readCounters: function readCounters(data) {

Could you take the opportunity to document this method?

@@ +96,1 @@
>  

Side-note: The lines below this don't look like reading to me. Perhaps they would be better in another method?

@@ +102,5 @@
>      },
>  
> +    get _dataFile() {
> +        delete this._dataFile;
> +        return this._dataFile = OS.Path.join(OS.Constants.Path.profileDir, "httpDataUsage.dat");

Since you return a path rather than a file, I'd suggest calling this |_dataPath|. Also, please document.

@@ +110,1 @@
>      writeCounters: function writeCounters() {

Please document the return value.

@@ +116,5 @@
>  
> +        let encoder = new TextEncoder();
> +        let array = encoder.encode(dataout);
> +        let writePromise = OS.File.writeAtomic(this._dataFile, array,
> +                                               {tmpPath: this._dataFile + ".tmp"});

You don't need the encoder, just pass a string to writeAtomic.

@@ +125,3 @@
>      },
>  
>      submitTelemetry: function submitTelemetry() {

Since you're now returning something, please document the return value.

@@ +163,3 @@
>      },
>  
> +    _updatePending: false,

Please document. You have e.g. an updatePending and an updatePromise that don't have the same meaning for "update".

@@ +163,5 @@
>      },
>  
> +    _updatePending: false,
> +
> +    updateStats: function updateStats() {

Please document, in particular the return value – and the relationship with |updateStatsAgain|.

@@ +166,5 @@
> +
> +    updateStats: function updateStats() {
> +        return Task.spawn(function *() {
> +            this._updatePending = true;
> +            let decoder = new TextDecoder();

Note: This decoder is will stop being necessary once bug 961665 lands.

@@ +179,5 @@
> +            }
> +            this.readCounters(data);
> +            yield this.submitTelemetry();
> +        }.bind(this)).then(
> +            () => this._updatePending = false,

So you're canceling _updatePending only in case of success? I would have imagined that you would cancel it whenever you reach the end.

Also, since you're using Task.spawn, you should use a try/catch/finally instead of a |then|.

@@ +187,5 @@
>  
> +    // Promise that is resolved when any pending updateStats tasks are complete
> +    _updatePromise: null,
> +
> +    updateStatsAgain: function () {

Please find a better name.
Attachment #8380244 - Flags: review?(dteller) → feedback+
Comment on attachment 8380244 [details] [diff] [review]
patch

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

Gavin - thanks for the patch. I have a couple questions for my own edification I will follow up with later.

Coincidentally I have a todo list to remove this code anyhow - it has served its purpose and is being eol'd now. So I'll just write a patch to yank it out in the next 36 hours or so. Let me know if that isn't acceptable.
Attachment #8380244 - Flags: review?(mcmanus)
Removing it entirely sounds great, feel free to use this bug for that!
Assignee: gavin.sharp → nobody
Attachment #8381733 - Flags: review?(jduell.mcbugs)
Assignee: nobody → mcmanus
https://tbpl.mozilla.org/?tree=Try&rev=062035a703c0

jason - I'll summarize data learned from this elsewhere when I get a chance.. but lets get the patch done.
Comment on attachment 8381733 [details] [diff] [review]
remove httpdatausage

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

::: netwerk/protocol/http/nsHttpConnection.h
@@ +211,5 @@
>      nsresult StartShortLivedTCPKeepalives();
>      nsresult StartLongLivedTCPKeepalives();
>      nsresult DisableTCPKeepalives();
>  
> +private:

Doesn't look like you need this--scope is already private.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1910,4 @@
>          return;
>  
> +    // If B2G requires a similar mechanism nsINetworkManager, currently only avail
> +    // on B2G, contains the necessary information on wifi and gateway

Would be nice to point some B2G testers at your zap test to make sure we don't need tickling on B2G.  Seems unlikely, but it would be a huge deal if we need it and don't.
Attachment #8381733 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/a4c51a14aaf2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.