Bug 632177 shows that using the system's mimetypes can be very expensive for Fennec. This gives mimetype hints for JSON in sync.
I haven't set myself up to make sure this fixes the problem on Android. Matt or Taras, mind testing?
Attachment #510634 - Flags: review?(edilee)
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?)
Attachment #510634 - Flags: review?(edilee) → review?(philipp)
Any idea how content type lookup works? The OS will separately synchronously read the file to infer a type?
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."
I have verified that this eliminates the calls from Weave to GetMIMEInfoFromOS in Fennec.
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.
Attachment #510634 - Flags: review?(philipp)
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)
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, ...
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
(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.
Component: Web Services → Firefox Sync: Backend
Product: Core → Mozilla Services
QA Contact: web-services → sync-backend
Version: Trunk → unspecified
Attachment #510634 - Attachment is obsolete: true
Attachment #510707 - Flags: review?(philipp)
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/
Attachment #510707 - Flags: review?(philipp) → review+
Attachment #510707 - Attachment is obsolete: true
This should probably block Fennec if it provides the suggested performance improvement.
tracking-fennec: --- → ?
Yes, it does. We have seen a Ts improvement from similar patches that already landed.
tracking-fennec: ? → 2.0b5+
Landed in fx-sync: https://hg.mozilla.org/services/fx-sync/rev/20b5aa56b69f
Whiteboard: [fixed in fx-sync]
Landed in places: http://hg.mozilla.org/projects/places/rev/8214fee0a355
Whiteboard: [fixed in fx-sync] → [fixed in fx-sync][fixed in places]
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?
(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.
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/8214fee0a355
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-sync][fixed in places]
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.