Closed Bug 632417 Opened 11 years ago Closed 11 years ago
Use mimetype hint for JSON in sync
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
Closed: 11 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.