632417 - Use mimetype hint for JSON in sync

Status: RESOLVED FIXED

Description

[Benjamin Stover (:stechz)]

Bug 632177 shows that using the system's mimetypes can be very expensive for Fennec. This gives mimetype hints for JSON in sync.

Comment 1

[Benjamin Stover (:stechz)]

Created attachment 510634 [details] [diff] [review]
Use mimetype hint for JSON in sync

Comment 2

[Benjamin Stover (:stechz)]

I haven't set myself up to make sure this fixes the problem on Android. Matt or Taras, mind testing?

Comment 3

[Ed Lee :Mardak]

Comment on attachment 510634 [details] [diff] [review]
Use mimetype hint for JSON in sync

Seems reasonable. Passing review to sync team.

Will need to get a reviewer for netutil bits. Maybe sdwilsh or bz? (I'm assuming setting the content type will prevent future content type lookups?)

Comment 4

[Ed Lee :Mardak]

Any idea how content type lookup works? The OS will separately synchronously read the file to infer a type?

Comment 5

[Benjamin Stover (:stechz)]

See: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/file/nsFileChannel.cpp#297

And: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsBaseChannel.cpp#182

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 510634 [details] [diff] [review]
Use mimetype hint for JSON in sync

>+     * @param aHint
>+     *        The mimetype of the resource we are opening (optional)

Drive-by review: I would prefer a more specific identifier like "aContentType" or "aTypeHint."

Comment 7

[Matt Brubeck (:mbrubeck)]

I have verified that this eliminates the calls from Weave to GetMIMEInfoFromOS in Fennec.

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 510634 [details] [diff] [review]
Use mimetype hint for JSON in sync

Looks good to me, but please create a separate patch for the Sync bit since it needs to land in https://hg.mozilla.org/services/fx-sync/ (the canonical repository, merged periodically). Clearing the review flag for now.

Btw, if there's a way to test the NetUtil bit (I imagine there is), there should be a test exercising the new argument.

Comment 9

[Ed Lee :Mardak]

For the test, I was thinking it might be tricky to test that GetMIMEInfoFromOS doesn't get called. But I think a satisfactory test would be to provide an override. E.g., text/plain for a application/json file. (But I'm not a network peer! :p)

Comment 10

[Shawn Wilsher :sdwilsh]

I'm not sure we actually want to make this change to NetUtil (and this isn't in a component that people who actually care about this stuff would see it).  It'd be pretty trivial for the Sync code to just say:
let channel = NetUtil.newChannel(whateverItIsPassingInNow);
channel.setContentType("application/json");
NetUtil.asyncFetch(channel, ...

Comment 11

[Ed Lee :Mardak]

Oh hey indeed. It's passing in a nsIFile but NetUtil does expose newChannel, so what sdwilsh suggested should work without any NetUtils changes.

asyncFetch calling newChannel:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#177

Comment 12

[Philipp von Weitershausen [:philikon]]

(In reply to comment #10)
> let channel = NetUtil.newChannel(whateverItIsPassingInNow);
> channel.setContentType("application/json");
> NetUtil.asyncFetch(channel, ...

Good point. Moving this bug to Services :: Sync Backend.

Comment 13

[Benjamin Stover (:stechz)]

Created attachment 510707 [details] [diff] [review]
Use mimetype hint for JSON in sync

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 510707 [details] [diff] [review]
Use mimetype hint for JSON in sync

Please land this in https://hg.mozilla.org/services/fx-sync/

Comment 15

[Benjamin Stover (:stechz)]

Created attachment 510761 [details] [diff] [review]
Use mimetype hint for JSON in sync

Comment 16

[Philipp von Weitershausen [:philikon]]

This should probably block Fennec if it provides the suggested performance improvement.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Yes, it does. We have seen a Ts improvement from similar patches that already landed.

Comment 18

[Philipp von Weitershausen [:philikon]]

Landed in fx-sync: https://hg.mozilla.org/services/fx-sync/rev/20b5aa56b69f

Comment 19

[Philipp von Weitershausen [:philikon]]

Landed in places: http://hg.mozilla.org/projects/places/rev/8214fee0a355

Comment 20

[Benjamin Stover (:stechz)]

Hm, there were some issues in try I wanted to look at, but it seems that it has landed fine in places. Shall we commit this to m-c as well?

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to comment #20)
> Hm, there were some issues in try I wanted to look at,

http://tbpl.mozilla.org/?tree=MozillaTry&rev=878c925f96ea looks good to me.

> Shall we commit this to m-c as well?

I will merge places to m-c later today once the tree reopens.

Comment 22

[Philipp von Weitershausen [:philikon]]

Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/8214fee0a355