Closed
Bug 1045503
Opened 10 years ago
Closed 10 years ago
[tarako][contacts] we should reduce the use of storage by contacts app
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)
People
(Reporter: wchang, Assigned: arcturus)
Details
Attachments
(2 files)
46 bytes,
text/x-github-pull-request
|
jmcf
:
review+
bajaj
:
approval-gaia-v2.0+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
timdream
:
review+
bajaj
:
approval-gaia-v1.4+
|
Details | Review |
As the tarako device has a small internal storage partition we should reduce the storage uses by our core apps, contacts especially seem to be using quite a bit.
Reporter | ||
Comment 1•10 years ago
|
||
ni? KaiZhen as he has some stats on the storage used by importing contacts. Steve, can you also share your analysis here? Thanks
Flags: needinfo?(schung)
Flags: needinfo?(kli)
Comment 2•10 years ago
|
||
After imported contact from facebook and gmail, I noticed that it consumes about 31MB from /data/local. $ du -sh local/storage/persistent/* 192K local/storage/persistent/1003+f+app+++video.gaiamobile.org 48K local/storage/persistent/1005+f+app+++wappush.gaiamobile.org 1.7M local/storage/persistent/1006+f+app+++gallery.gaiamobile.org 560K local/storage/persistent/1008+f+app+++homescreen.gaiamobile.org 48K local/storage/persistent/1010+f+app+++costcontrol.gaiamobile.org 48K local/storage/persistent/1011+f+app+++calendar.gaiamobile.org 48K local/storage/persistent/1012+f+app+++callscreen.gaiamobile.org 48K local/storage/persistent/1013+f+app+++camera.gaiamobile.org 192K local/storage/persistent/1014+f+app+++email.gaiamobile.org 88K local/storage/persistent/1015+f+app+++communications.gaiamobile.org 11M local/storage/persistent/1016+f+app+++music.gaiamobile.org 236K local/storage/persistent/1017+f+app+++browser.gaiamobile.org 88K local/storage/persistent/1018+f+app+++clock.gaiamobile.org 48K local/storage/persistent/1019+f+app+++sms.gaiamobile.org 48K local/storage/persistent/1022+f+app+++system.gaiamobile.org 88K local/storage/persistent/1024+f+app+++marketplace.firefox.com 84K local/storage/persistent/1031+f+app+++{f7f75813-8f7a-4a3f-87b0-093662a3dfa9} 31M local/storage/persistent/chrome
Flags: needinfo?(kli)
Comment 3•10 years ago
|
||
Note that "local/storage/persistent/chrome" contains several DB, you should go inside the directory and look at the sizes of the various DB.
Updated•10 years ago
|
Flags: needinfo?(kli)
Comment 4•10 years ago
|
||
Wayne and Kai-Zhen, may I know the test environment for the contact app? How many contacts did you added/imported for the testing?
Flags: needinfo?(schung)
Comment 5•10 years ago
|
||
Before import contacts, local/storage/persistent/chrome consumed only about 500K. After imported contact from facebook and gmail, it is increased to about 31MB. It is about 600 from facebook and 200 from gmail. Most from gmail is without picture. $ du -sh local/storage/persistent/chrome/idb/* 48K local/storage/persistent/chrome/idb/226660312ssm.sqlite 0 local/storage/persistent/chrome/idb/226660312ssm.sqlite-journal 172K local/storage/persistent/chrome/idb/2588645841ssegtnti 100K local/storage/persistent/chrome/idb/2588645841ssegtnti.sqlite 0 local/storage/persistent/chrome/idb/2588645841ssegtnti.sqlite-journal 80K local/storage/persistent/chrome/idb/3104902905ascetiitvi.sqlite 0 local/storage/persistent/chrome/idb/3104902905ascetiitvi.sqlite-journal 40K local/storage/persistent/chrome/idb/3249156127nsetta_ts.sqlite 0 local/storage/persistent/chrome/idb/3249156127nsetta_ts.sqlite-journal 48K local/storage/persistent/chrome/idb/3406066227csotncta 2.6M local/storage/persistent/chrome/idb/3406066227csotncta.sqlite 0 local/storage/persistent/chrome/idb/3406066227csotncta.sqlite-journal 44K local/storage/persistent/chrome/idb/4045445992aslmar.sqlite 0 local/storage/persistent/chrome/idb/4045445992aslmar.sqlite-journal 27M local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo 1.2M local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo.sqlite 0 local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo.sqlite-journal 40K local/storage/persistent/chrome/idb/846562544phus.sqlite 0 local/storage/persistent/chrome/idb/846562544phus.sqlite-journal
Flags: needinfo?(kli)
Comment 6•10 years ago
|
||
Hey Francisco, I've discovered that contact photo imported from facebook was save as png type. It seems image downloaded from server is PNG by default and it will take much more memory then jpg by default. Maybe we can convert to jpg before we saved to DB, wdyt?
Flags: needinfo?(francisco)
Reporter | ||
Comment 7•10 years ago
|
||
Echoing Steve - not sure if services like facebook sync provides jpg download also? If so we can take that option easier...
Comment 8•10 years ago
|
||
48K local/storage/persistent/chrome/idb/3406066227csotncta 2.6M local/storage/persistent/chrome/idb/3406066227csotncta.sqlite => clearly, the contacts database is not too big (but probably because there are no pictures too) 27M local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo 1.2M local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo.sqlite => less data in the sqlite than for contacts (probably because there are less indices) => a lot more data in the attachments (pictures) Using jpg is likely a good idea :)
Reporter | ||
Comment 9•10 years ago
|
||
Flagging here for traction. If we can get this through the device would be a lot more usable for the end users.
blocking-b2g: --- → 1.3T+
Comment 10•10 years ago
|
||
Hi! Tim, Could someone of your team can take this case? Thanks -- Keven
Flags: needinfo?(timdream)
Comment 11•10 years ago
|
||
Steve will be available as fallback team if everyone else is working on more urgent stuff.
Flags: needinfo?(timdream)
Comment 12•10 years ago
|
||
BTW, Facebook does not indicate the format of the image they return nor there is a parameter available for explicitly specifying the format. https://developers.facebook.com/docs/graph-api/reference/v2.0/user/picture From my experiment Facebook always returns JPEG (try the URL below for example). http://graph.facebook.com/mozilla/picture I don't know why we are getting PNG in the storage in this function since we are indeed using the same API. https://github.com/mozilla-b2g/gaia/blob/master/shared/js/contacts/import/facebook/fb_query.js#L94-L139
Comment 14•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #13) > Maybe we're using canvas' toBlob or toDataURL which uses png by default. I did grep these two function names and didn't find them :'(
Comment 15•10 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #13) > Maybe we're using canvas' toBlob or toDataURL which uses png by default. I found that we call "toBlob" here: https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/communications/contacts/js/utilities/image_thumbnail.js#L46
Comment 16•10 years ago
|
||
Hey Francisco, we found toBlob return png by default, so we must force the mime type for conversion. Please also down scale the image quality in image_square.js because image download from facebook seems use lower quality then toBlob default(0.95), thanks!
Assignee: nobody → francisco
Assignee | ||
Comment 17•10 years ago
|
||
Hei Steve, thanks for diving into to this. Will try to provide a patch ASAP.
Flags: needinfo?(francisco)
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → 2.1 S1 (1aug)
Comment 19•10 years ago
|
||
Sorry to jump in. Due to the urgency of Tarako, I'm helping to uplift Francisco's patch to v1.3t so QA can be involved ASAP. I'll back it out if by any chance the original patch get r-. In addition, I also decreased the quality of the "jpeg" compression in the patch since we have to reduce the storage consumption on Tarako as much as possible. Tim, could you please help to review this Tarako-only patch. Thanks.
Attachment #8465184 -
Flags: review?(timdream)
Comment 20•10 years ago
|
||
Set qawanted for attachment 8465184 [details] [review]. Thanks.
Keywords: qawanted
Comment 21•10 years ago
|
||
Comment on attachment 8465184 [details] [review] Pull Request for v1.3t rubberstamping -- this does not affect review of patch on master. Please make a note here on the quality parameter used (0.65).
Attachment #8465184 -
Flags: review?(timdream) → review+
Comment 22•10 years ago
|
||
Before jpg is used for import contact from facebook occupied about 28MB. After jpg is use, I can see the application storage used by contact is decreased. So I also suggest to use 0.65 on master, as the image quality is also acceptable and it use less storage. The image quality of 0.35 is blurry and the storage is not saved much than 0.65. -- 0.95 18M local/storage/persistent/chrome 0.65 12M local/storage/persistent/chrome 8.6M local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo 1.3M local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo.sqlite 0.35 12M local/storage/persistent/chrome 8.0M local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo 1.2M local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo.sqlite
Comment 23•10 years ago
|
||
ni? Mike for him to verify with the patch provided by Luke.
Flags: needinfo?(mlien)
Comment 24•10 years ago
|
||
Thanks Kai-zhen for pasting testing result of previous patch. We will let QA test the patch as well, and land 1.3T patch if there is no problem found.
Comment 26•10 years ago
|
||
I think we also need this on master.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 27•10 years ago
|
||
Comment on attachment 8464682 [details] [review] Pointer to PR 22311 trivial change. r+ from my side and I left to the implementor the decision of what quality params to be used
Attachment #8464682 -
Flags: review?(jmcf) → review+
Comment 28•10 years ago
|
||
verify patch with 350 FB contacts + 100 outlook contacts reduced usage of storage near 15MB original: 40K data/local/storage/persistent/chrome/idb/226660312ssm.sqlite 0 data/local/storage/persistent/chrome/idb/226660312ssm.sqlite-journal 172K data/local/storage/persistent/chrome/idb/2588645841ssegtnti 96K data/local/storage/persistent/chrome/idb/2588645841ssegtnti.sqlite 0 data/local/storage/persistent/chrome/idb/2588645841ssegtnti.sqlite-journal 72K data/local/storage/persistent/chrome/idb/3104902905ascetiitvi.sqlite 0 data/local/storage/persistent/chrome/idb/3104902905ascetiitvi.sqlite-journal 40K data/local/storage/persistent/chrome/idb/3249156127nsetta_ts.sqlite 0 data/local/storage/persistent/chrome/idb/3249156127nsetta_ts.sqlite-journal 3.8M data/local/storage/persistent/chrome/idb/3406066227csotncta 1.5M data/local/storage/persistent/chrome/idb/3406066227csotncta.sqlite 0 data/local/storage/persistent/chrome/idb/3406066227csotncta.sqlite-journal 40K data/local/storage/persistent/chrome/idb/4045445992aslmar.sqlite 0 data/local/storage/persistent/chrome/idb/4045445992aslmar.sqlite-journal 17M data/local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo 648K data/local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo.sqlite 0 data/local/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo.sqlite-journal 40K data/local/storage/persistent/chrome/idb/846562544phus.sqlite 0 data/local/storage/persistent/chrome/idb/846562544phus.sqlite-journal with patch: 40K datalocal/storage/persistent/chrome/idb/226660312ssm.sqlite 0 datalocal/storage/persistent/chrome/idb/226660312ssm.sqlite-journal 172K datalocal/storage/persistent/chrome/idb/2588645841ssegtnti 96K datalocal/storage/persistent/chrome/idb/2588645841ssegtnti.sqlite 0 datalocal/storage/persistent/chrome/idb/2588645841ssegtnti.sqlite-journal 76K datalocal/storage/persistent/chrome/idb/3104902905ascetiitvi.sqlite 0 datalocal/storage/persistent/chrome/idb/3104902905ascetiitvi.sqlite-journal 40K datalocal/storage/persistent/chrome/idb/3249156127nsetta_ts.sqlite 0 datalocal/storage/persistent/chrome/idb/3249156127nsetta_ts.sqlite-journal 660K datalocal/storage/persistent/chrome/idb/3406066227csotncta 1.5M datalocal/storage/persistent/chrome/idb/3406066227csotncta.sqlite 0 datalocal/storage/persistent/chrome/idb/3406066227csotncta.sqlite-journal 40K datalocal/storage/persistent/chrome/idb/4045445992aslmar.sqlite 0 datalocal/storage/persistent/chrome/idb/4045445992aslmar.sqlite-journal 4.6M datalocal/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo 656K datalocal/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo.sqlite 0 datalocal/storage/persistent/chrome/idb/4272465303Gpapiaab_eFwa.ctesbeo.sqlite-journal 40K datalocal/storage/persistent/chrome/idb/846562544phus.sqlite 0 datalocal/storage/persistent/chrome/idb/846562544phus.sqlite-journal
Flags: needinfo?(mlien)
Assignee | ||
Comment 29•10 years ago
|
||
Landed on master: https://github.com/mozilla-b2g/gaia/commit/464599a069d9fed208dec90692c66e91ffc070ea Thanks everyone involved!
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 30•10 years ago
|
||
Francisco, the improvement is so big for a so safe and simple change, that I think we should uplift to v2.0. I let you request an approval?
Flags: needinfo?(francisco)
Assignee | ||
Comment 31•10 years ago
|
||
Hi Julien, You are definitely right, will request the uplift with current quality 0.95. Thanks for the remind!
Flags: needinfo?(francisco)
Assignee | ||
Updated•10 years ago
|
status-b2g-v2.0:
--- → affected
Assignee | ||
Comment 32•10 years ago
|
||
Comment on attachment 8464682 [details] [review] Pointer to PR 22311 NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] [Bug caused by] We are storing png images instead of compressed jpeg ones [User impact] We are consuming too much internal memory and that affects the space available for the user. [Testing completed]: Tested on master, also this patch has been uplift to 1.3T [Risk to taking this patch] (and alternatives if risky): Minimal risk, we are just requesting the platform a jpeg format instead of the png one [String changes made]:
Attachment #8464682 -
Flags: approval-gaia-v2.0?
Comment 33•10 years ago
|
||
Comment on attachment 8464682 [details] [review] Pointer to PR 22311 Given the super low risk and the internal memory wins here, we'll take this on 2.0..Any fallouts/regressions this causes, please backout immediately.
Attachment #8464682 -
Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Comment 34•10 years ago
|
||
v2.0: https://github.com/mozilla-b2g/gaia/commit/b3675c8e55e2b784b5e5dbb23df6bd281f85732a Worth considering for Dolphin on v1.4 as well?
Assignee | ||
Comment 35•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #34) > v2.0: > https://github.com/mozilla-b2g/gaia/commit/ > b3675c8e55e2b784b5e5dbb23df6bd281f85732a > > Worth considering for Dolphin on v1.4 as well? Definitely, should I ask for the flags?
Flags: needinfo?(francisco)
Reporter | ||
Comment 37•10 years ago
|
||
Agree that this is needed for 1.4. Instead of flagging we should request for approval. Francisco, Can you request for 1.4 approval on the patch if it'll land directly? Thanks
blocking-b2g: 1.4? → 1.3T+
Flags: needinfo?(francisco)
Assignee | ||
Comment 38•10 years ago
|
||
Comment on attachment 8465184 [details] [review] Pull Request for v1.3t NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] [Bug caused by] Not a bug, but a feature for using less space on images downloaded. [User impact] if declined: A lot of usage in internal memory, not allowing the user to use the phone for other stuff like photos, music, etc. [Testing completed]: Yes, in versions 1.3 and 2.0 [Risk to taking this patch] (and alternatives if risky): Pretty low risk, since we are just saying gecko to compress the images when we save them. [String changes made]:
Attachment #8465184 -
Flags: approval-gaia-v1.4?
Flags: needinfo?(francisco)
Updated•10 years ago
|
Attachment #8465184 -
Flags: approval-gaia-v1.4? → approval-gaia-v1.4+
Comment 39•10 years ago
|
||
v1.4: https://github.com/mozilla-b2g/gaia/commit/4d5451815fbab514fbdfed4268d9f683e4e05bb4
You need to log in
before you can comment on or make changes to this bug.
Description
•