:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Description • 11 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 1 • 11 years ago

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).

	David Teller [:Yoric] - still alive but not very active
	Comment 2 • 11 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 3 • 11 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 4 • 11 years ago

Removing it entirely sounds great, feel free to use this bug for that!

	Patrick McManus [:mcmanus] Assignee
	Comment 6 • 11 years ago

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.

	Jason Duell
	Comment 7 • 11 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 8 • 11 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c51a14aaf2

	Carsten Book [:Tomcat]
	Comment 9 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/a4c51a14aaf2