Closed
Bug 767025
Opened 13 years ago
Closed 13 years ago
Store activity flags on offline cache entries in memory for better performance
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
People
(Reporter: glandium, Assigned: sinker)
References
Details
Attachments
(5 files, 16 obsolete files)
7.13 KB,
text/plain
|
Details | |
27.92 KB,
text/plain
|
Details | |
16.37 KB,
patch
|
sinker
:
review+
|
Details | Diff | Splinter Review |
10.98 KB,
patch
|
sinker
:
review+
|
Details | Diff | Splinter Review |
9.14 KB,
patch
|
sinker
:
review+
|
Details | Diff | Splinter Review |
Accesses to the offlinecache by b2g triggers writes to the index.sqlite journal. This obviously shouldn't happen when the cache is populated and when what is being looked for is already there.
![]() |
||
Comment 1•13 years ago
|
||
We are marking active items (=being currently held by some channel) in the database. I think this is the source of writes. This way of marking items directly in the database has chosen to make queries easier to build and be more optimal from point of view of SQL.
We could have an in-memory view or joined in-memory table that would hold this info and still could be efficiently used in queries.
![]() |
||
Comment 2•13 years ago
|
||
We do UPDATE moz_cache SET Flags = 1 when an entry is activated and drop this back to zero when it is no longer active. On start we are also dropping this flag from the database. Clearly a candidate to move to an in-memory table.
Reporter | ||
Comment 3•13 years ago
|
||
Relatedly, the database is not in WAL mode. Switching it in WAL mode already does make things faster, although not as fast as it would be with an in-memory table.
Reporter | ||
Comment 4•13 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #3)
> Relatedly, the database is not in WAL mode. Switching it in WAL mode already
> does make things faster, although not as fast as it would be with an
> in-memory table.
Mmmm apparently, switching OfflineCache/index.sqlite to WAL mode crashes b2g :-/
Reporter | ||
Comment 5•13 years ago
|
||
This is the mozStorage log I get when launching the B2G Template app:
074324840[11950]: sqlite3_trace on 8b8920 for 'SELECT ClientID, ItemType FROM moz_cache WHERE Key = 'http://template.gaiamobile.org/' ORDER BY LastFetched DESC, LastModified DESC;'
980744[890860]: sqlite3_trace on 8b8920 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340279803|25' AND Key = 'http://template.gaiamobile.org/';'
980744[890860]: sqlite3_trace on 8b8920 for 'UPDATE moz_cache SET Flags = 1 WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340279803|25' AND Key = 'http://template.gaiamobile.org/';'
980744[890860]: sqlite3_trace on 8b8920 for 'UPDATE moz_cache SET MetaData = x'726571756573742d6d6574686f640047455400726573706f6e73652d6865616400474554202f20485454502f312e310d0a00636861727365740049534f2d383835392d3100', Flags = 0, DataSize = 286, FetchCount = 2, LastFetched = 1340342338000000, LastModified = 1340342338000000, ExpirationTime = 0 WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340279803|25' AND Key = 'http://template.gaiamobile.org/';'
980744[890860]: sqlite3_trace on 8b8920 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340279803|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
980744[890860]: sqlite3_trace on 8b8920 for 'UPDATE moz_cache SET Flags = 1 WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340279803|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
980744[890860]: sqlite3_trace on 8b8920 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340342338|15' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
074324840[11950]: sqlite3_trace on 8b8920 for 'UPDATE moz_cache SET MetaData = x'726571756573742d6d6574686f640047455400726573706f6e73652d6865616400474554202f20485454502f312e310d0a00', Flags = 0, DataSize = 224, FetchCount = 2, LastFetched = 1340342338000000, LastModified = 1340279803000000, ExpirationTime = 0 WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340279803|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
074324840[11950]: sqlite3_trace on 8b8920 for 'CREATE TEMP TRIGGER cache_on_delete AFTER DELETE ON moz_cache FOR EACH ROW BEGIN SELECT cache_eviction_observer( OLD.key, OLD.generation); END;'
074324840[11950]: Initialized statement 'DELETE FROM moz_cache WHERE ClientID=? AND Flags = 0;' (0x814ee0)
074324840[11950]: sqlite3_trace on 8b8920 for 'DELETE FROM moz_cache WHERE ClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340342338|15' AND Flags = 0;'
074324840[11950]: Finalizing statement 'DELETE FROM moz_cache WHERE ClientID=? AND Flags = 0;'
074324840[11950]: Initialized statement 'DELETE FROM moz_cache_groups WHERE ActiveClientID=?;' (0x814ee0)
074324840[11950]: sqlite3_trace on 8b8920 for 'DELETE FROM moz_cache_groups WHERE ActiveClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340342338|15';'
074324840[11950]: Finalizing statement 'DELETE FROM moz_cache_groups WHERE ActiveClientID=?;'
074324840[11950]: Initialized statement 'DELETE FROM moz_cache_namespaces WHERE ClientID=?' (0x814ee0)
074324840[11950]: sqlite3_trace on 8b8920 for 'DELETE FROM moz_cache_namespaces WHERE ClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340342338|15''
074324840[11950]: Finalizing statement 'DELETE FROM moz_cache_namespaces WHERE ClientID=?'
074324840[11950]: sqlite3_trace on 8b8920 for 'DROP TRIGGER cache_on_delete;'
980744[890860]: sqlite3_trace on 8b8920 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340279803|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
980744[890860]: sqlite3_trace on 8b8920 for 'UPDATE moz_cache SET Flags = 1 WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340279803|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
980744[890860]: sqlite3_trace on 8b8920 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340342338|16' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
074324840[11950]: sqlite3_trace on 8b8920 for 'UPDATE moz_cache SET MetaData = x'726571756573742d6d6574686f640047455400726573706f6e73652d6865616400474554202f20485454502f312e310d0a00', Flags = 0, DataSize = 224, FetchCount = 3, LastFetched = 1340342338000000, LastModified = 1340279803000000, ExpirationTime = 0 WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340279803|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
074324840[11950]: sqlite3_trace on 8b8920 for 'CREATE TEMP TRIGGER cache_on_delete AFTER DELETE ON moz_cache FOR EACH ROW BEGIN SELECT cache_eviction_observer( OLD.key, OLD.generation); END;'
074324840[11950]: Initialized statement 'DELETE FROM moz_cache WHERE ClientID=? AND Flags = 0;' (0x814ee0)
074324840[11950]: sqlite3_trace on 8b8920 for 'DELETE FROM moz_cache WHERE ClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340342338|16' AND Flags = 0;'
074324840[11950]: Finalizing statement 'DELETE FROM moz_cache WHERE ClientID=? AND Flags = 0;'
074324840[11950]: Initialized statement 'DELETE FROM moz_cache_groups WHERE ActiveClientID=?;' (0x814ee0)
074324840[11950]: sqlite3_trace on 8b8920 for 'DELETE FROM moz_cache_groups WHERE ActiveClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340342338|16';'
074324840[11950]: Finalizing statement 'DELETE FROM moz_cache_groups WHERE ActiveClientID=?;'
074324840[11950]: Initialized statement 'DELETE FROM moz_cache_namespaces WHERE ClientID=?' (0x814ee0)
074324840[11950]: sqlite3_trace on 8b8920 for 'DELETE FROM moz_cache_namespaces WHERE ClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340342338|16''
074324840[11950]: Finalizing statement 'DELETE FROM moz_cache_namespaces WHERE ClientID=?'
074324840[11950]: sqlite3_trace on 8b8920 for 'DROP TRIGGER cache_on_delete;'
Reporter | ||
Comment 6•13 years ago
|
||
Here is a testcase showing how hurtful this can be:
- In B2G on an otoro phone, open the dialer
- Type a phone number, say, 555
- Tap the green call button
- The call info panel shows up after ~5s (no matter how many subsequent times you try).
Now, a little hack to make the database updates virtually free:
- adb shell
$ stop b2g
$ mv /data/local/OfflineCache /data/local/OfflineCache.orig
$ mkdir /data/local/OfflineCache
$ mount -t tmpfs none /data/local/OfflineCache
$ for i in 0 1 2 3 4 5 6 7 8 9 A B C D E F; do ln -s /data/local/OfflineCache.orig/$i /data/local/OfflineCache/$i; done
$ cat /data/local/OfflineCache.orig/index.sqlite > /data/local/OfflineCache/index.sqlite
$ start b2g
(In practice, this puts the OfflineCache index.sqlite database on tmpfs, but not the data itself ; this also guarantees that sqlite temporary (journal) files are created on the tmpfs)
- Try again. The call info panel now shows up after ~1s.
Honza, do you think you can take this bug and improve the situation? I'm not familiar with the code, and it would make sense that someone that is takes it further.
![]() |
||
Comment 7•13 years ago
|
||
Thanks Mike for confirming my words. How important is this (deadline) ?
Assignee: nobody → honzab.moz
Summary: Investigate why offlinecache triggers writes when reading from the cache → Store activity flags on offline cache entries in memory for better performance
![]() |
||
Updated•13 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•13 years ago
|
||
if we go to multi-processing, is it still working? (in memory) Or we should involve shared memory!?
Content processes don't know about offlinecache, AFAIK. The requests should be routed through the master process.
![]() |
||
Comment 10•13 years ago
|
||
Yes, access to the sqlite DB is made only on the chrome process. This will "just work".
Comment 11•13 years ago
|
||
This is of critical importance to fix ASAP. It makes our performance miserable. Can we parallelize reads and remove the writes? Do you have an ETA for a patch?
Assignee | ||
Comment 12•13 years ago
|
||
Is there any reason that we should query database for the cache entry every time? Can we just hash the path and check directories for the file with the hashed name. We can create a directory for each version of apps to collect all files. So, just query for the directory of the latest version one time, and cache it on the memory. I think it would enhance more.
![]() |
||
Comment 13•13 years ago
|
||
Thinker, I really don't understand what you mean. To clarify: an entry is active when a channel(s) is(are) currently working with it (i.e. writes to it, or read it). Entries are usually active for a short time. The active indication is used to prevent race conditions and leave entries the platform currently works with alone when e.g. evicting entries.
Assignee | ||
Comment 14•13 years ago
|
||
Honza, I known it. I am off the topic. I am talking about if it is possible to avoid also the queries for every cache entry. I mean mStatement_FindEntry. Maybe I should create a new bug for it.
![]() |
||
Comment 15•13 years ago
|
||
(In reply to Thinker Li [:sinker] from comment #14)
> Honza, I known it. I am off the topic. I am talking about if it is
> possible to avoid also the queries for every cache entry. I mean
> mStatement_FindEntry. Maybe I should create a new bug for it.
Ah, sorry, I was confused. That should be in a different bug, please CC me.
![]() |
||
Comment 16•13 years ago
|
||
Attachment #636918 -
Flags: review?(dcamp)
Comment 17•13 years ago
|
||
Comment on attachment 636918 [details] [diff] [review]
v1
Review of attachment 636918 [details] [diff] [review]:
-----------------------------------------------------------------
If the process crashes while there are multiple versions of the cache, won't you lose which one is active?
Comment 18•13 years ago
|
||
(In reply to Dave Camp (:dcamp) from comment #17)
> If the process crashes while there are multiple versions of the cache, won't
> you lose which one is active?
Ignore that question, I was misremembering what that flag was about.
Comment 19•13 years ago
|
||
So this patch will reduce write churn, but we'll still be updating LastFetched and FetchedCount for each entry. Are those actually useful? Can we stop storing those entirely, or at least batch updates to them?
Reporter | ||
Comment 20•13 years ago
|
||
It doesn't look like it does much difference on the dialer testcase.
With this patch, launching the template application looks like this:
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'SELECT ClientID, ItemType FROM moz_cache WHERE Key = 'http://template.gaiamobile.org/' ORDER BY LastFetched DESC, LastModified DESC;'
I/PRLog ( 299): 39734488[25e4c60]: sqlite3_trace on 20c1400 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25' AND Key = 'http://template.gaiamobile.org/' AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );'
I/PRLog ( 299): 39734488[25e4c60]: sqlite3_trace on 20c1400 for 'INSERT OR REPLACE INTO moz_cache_active VALUES ('http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25', 'http://template.gaiamobile.org/');'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'UPDATE moz_cache SET MetaData = x'726571756573742d6d6574686f640047455400726573706f6e73652d6865616400474554202f20485454502f312e310d0a00636861727365740049534f2d383835392d3100', Flags = 0, DataSize = 286, FetchCount = 2, LastFetched = 1340778814000000, LastModified = 1340778814000000, ExpirationTime = 0 WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25' AND Key = 'http://template.gaiamobile.org/';'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DELETE FROM moz_cache_active WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25' AND Key = 'http://template.gaiamobile.org/';'
I/PRLog ( 299): 39734488[25e4c60]: sqlite3_trace on 20c1400 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache' AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );'
I/PRLog ( 299): 39734488[25e4c60]: sqlite3_trace on 20c1400 for 'INSERT OR REPLACE INTO moz_cache_active VALUES ('http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25', 'http://template.gaiamobile.org/manifest.appcache');'
I/PRLog ( 299): 39734488[25e4c60]: sqlite3_trace on 20c1400 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340778814|11' AND Key = 'http://template.gaiamobile.org/manifest.appcache' AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'UPDATE moz_cache SET MetaData = x'726571756573742d6d6574686f640047455400726573706f6e73652d6865616400474554202f20485454502f312e310d0a00', Flags = 0, DataSize = 224, FetchCount = 2, LastFetched = 1340778815000000, LastModified = 1340712152000000, ExpirationTime = 0 WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DELETE FROM moz_cache_active WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'CREATE TEMP TRIGGER cache_on_delete AFTER DELETE ON moz_cache FOR EACH ROW BEGIN SELECT cache_eviction_observer( OLD.key, OLD.generation); END;'
I/PRLog ( 299): 1074439528[1de4950]: Initialized statement 'DELETE FROM moz_cache WHERE ClientID = ? AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );' (0x275ebc0)
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DELETE FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340778814|11' AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );'
I/PRLog ( 299): 1074439528[1de4950]: Finalizing statement 'DELETE FROM moz_cache WHERE ClientID = ? AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );'
I/PRLog ( 299): 1074439528[1de4950]: Initialized statement 'DELETE FROM moz_cache_groups WHERE ActiveClientID=?;' (0x275ebc0)
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DELETE FROM moz_cache_groups WHERE ActiveClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340778814|11';'
I/PRLog ( 299): 1074439528[1de4950]: Finalizing statement 'DELETE FROM moz_cache_groups WHERE ActiveClientID=?;'
I/PRLog ( 299): 1074439528[1de4950]: Initialized statement 'DELETE FROM moz_cache_namespaces WHERE ClientID=?' (0x275ebc0)
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DELETE FROM moz_cache_namespaces WHERE ClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340778814|11''
I/PRLog ( 299): 1074439528[1de4950]: Finalizing statement 'DELETE FROM moz_cache_namespaces WHERE ClientID=?'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DROP TRIGGER cache_on_delete;'
I/PRLog ( 299): 39734488[25e4c60]: sqlite3_trace on 20c1400 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache' AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );'
I/PRLog ( 299): 39734488[25e4c60]: sqlite3_trace on 20c1400 for 'INSERT OR REPLACE INTO moz_cache_active VALUES ('http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25', 'http://template.gaiamobile.org/manifest.appcache');'
I/PRLog ( 299): 39734488[25e4c60]: sqlite3_trace on 20c1400 for 'SELECT MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340778814|12' AND Key = 'http://template.gaiamobile.org/manifest.appcache' AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'UPDATE moz_cache SET MetaData = x'726571756573742d6d6574686f640047455400726573706f6e73652d6865616400474554202f20485454502f312e310d0a00', Flags = 0, DataSize = 224, FetchCount = 3, LastFetched = 1340778815000000, LastModified = 1340712152000000, ExpirationTime = 0 WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DELETE FROM moz_cache_active WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340712152|25' AND Key = 'http://template.gaiamobile.org/manifest.appcache';'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'CREATE TEMP TRIGGER cache_on_delete AFTER DELETE ON moz_cache FOR EACH ROW BEGIN SELECT cache_eviction_observer( OLD.key, OLD.generation); END;'
I/PRLog ( 299): 1074439528[1de4950]: Initialized statement 'DELETE FROM moz_cache WHERE ClientID = ? AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );' (0x275ebc0)
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DELETE FROM moz_cache WHERE ClientID = 'http%3A//template.gaiamobile.org/manifest.appcache|0000001340778814|12' AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );'
I/PRLog ( 299): 1074439528[1de4950]: Finalizing statement 'DELETE FROM moz_cache WHERE ClientID = ? AND NOT EXISTS (SELECT * FROM moz_cache_active WHERE moz_cache.ClientID = moz_cache_active.ClientID AND moz_cache.Key = moz_cache_active.Key );'
I/PRLog ( 299): 1074439528[1de4950]: Initialized statement 'DELETE FROM moz_cache_groups WHERE ActiveClientID=?;' (0x275ebc0)
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DELETE FROM moz_cache_groups WHERE ActiveClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340778814|12';'
I/PRLog ( 299): 1074439528[1de4950]: Finalizing statement 'DELETE FROM moz_cache_groups WHERE ActiveClientID=?;'
I/PRLog ( 299): 1074439528[1de4950]: Initialized statement 'DELETE FROM moz_cache_namespaces WHERE ClientID=?' (0x275ebc0)
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DELETE FROM moz_cache_namespaces WHERE ClientID='http%3A//template.gaiamobile.org/manifest.appcache|0000001340778814|12''
I/PRLog ( 299): 1074439528[1de4950]: Finalizing statement 'DELETE FROM moz_cache_namespaces WHERE ClientID=?'
I/PRLog ( 299): 1074439528[1de4950]: sqlite3_trace on 20c1400 for 'DROP TRIGGER cache_on_delete;'
I can also attach the log for the dialer testcase if that's helpful.
Comment 21•13 years ago
|
||
LastFetched and FetchedCount seem to be used for about:cache only. We should nuke that. Honza why do we need the active flag? With the dooming we should never nuke open entries anyway. We should not do any writes during fetching, ever.
Comment 22•13 years ago
|
||
Also while we are at it. Can we use nsDiskCacheDevice instead of nsDiskCacheDeviceSQL for the offline cache? That might eliminate all problems as well.
Comment 23•13 years ago
|
||
Note: one of the reasons I am keen on avoiding the writes here is that we are sitting on a software write-leveled flash filesystem with dog slow write performance. Best case its slow, worst case we will keep the flash storage pretty quickly.
Comment 24•13 years ago
|
||
The right thing to do here is to delete the flag and the write-on-access fields. Rows should be locked ("mark active") in an in memory list which is then checked in the eviction observer and the observer can do a RAISE(IGNORE) to skip deleting that row.
Comment 25•13 years ago
|
||
Comment 26•13 years ago
|
||
Comment 27•13 years ago
|
||
Comment 28•13 years ago
|
||
The attached patches are completely untested but this is how I would do it. Please feel free to take the patches and run with it if you agree.
Assignee | ||
Comment 29•13 years ago
|
||
(In reply to Andreas Gal :gal from comment #22)
> Also while we are at it. Can we use nsDiskCacheDevice instead of
> nsDiskCacheDeviceSQL for the offline cache? That might eliminate all
> problems as well.
Offline cache requires versionize updating. A new version is active only after a full update. We still need some additional work on nsDiskCacheDevice to catch the requirements of offline cache.
Comment 30•13 years ago
|
||
Thinker, can you review my patches and see if they make sense? I don't know much about this code.
Comment 31•13 years ago
|
||
On a related note, for pages with a lot of resources we completely serialize access by doing sync sqlite queries here. This is really slow. A simple fix might be to fetch all resources with a single query and cache that and if the next fetch is from the same origin skip the query and look into the cache, otherwise evict the cache and cache the new origin. This might trash badly in corner cases but will be in almost all cases superior to what we do right now. I will talk to Thinker about this.
Reporter | ||
Comment 32•13 years ago
|
||
(In reply to Andreas Gal :gal from comment #25)
> Created attachment 637026 [details] [diff] [review]
> part 1: remove flags
I tried part 1 & 2, and now all I get is a neterror page because system.gaiamobile.org can't be found. Which may just be a problem with using an old database (since it's generated from an old xulrunner) with the new queries. This also suggests this would fail to work after upgrades on desktop.
Assignee | ||
Comment 33•13 years ago
|
||
fix some minor errors.
Attachment #637026 -
Attachment is obsolete: true
Assignee | ||
Comment 34•13 years ago
|
||
Raise ignore error correctly.
Attachment #637028 -
Attachment is obsolete: true
Assignee | ||
Comment 35•13 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #32)
> (In reply to Andreas Gal :gal from comment #25)
> > Created attachment 637026 [details] [diff] [review]
> > part 1: remove flags
>
> I tried part 1 & 2, and now all I get is a neterror page because
> system.gaiamobile.org can't be found. Which may just be a problem with using
> an old database (since it's generated from an old xulrunner) with the new
> queries. This also suggests this would fail to work after upgrades on
> desktop.
I have done some updates. Please ignore part 2, but apply part 1 and part 3. It seems work fine for me.
![]() |
||
Comment 36•13 years ago
|
||
As Thinker suggested we cannot easily use nsDiskCacheDevice. Btw, it writes last fetched and fetch count data too.
General question: is the cache read slow because of general read and write slowness of the device or just because of reads and writes to sqlite are slow?
Other question: if you create a patch that simply removes all these writes while fetching from the app cache (actually a simple unstable patch) does it help dramatically with the performance to prove sqlite writes are cause here?
And finally: if anyone wants to work on this, then please take this bug by changing the assignee field. I've spent a lot of time to produce the patch and what Andreas and Thinker made is more or less the same, so we shouldn't waste time this way.
I'll take a closer look at comment 20 that could be giving interesting information. Thanks for it, btw.
Reporter | ||
Comment 37•13 years ago
|
||
(In reply to Thinker Li [:sinker] from comment #35)
> (In reply to Mike Hommey [:glandium] from comment #32)
> > (In reply to Andreas Gal :gal from comment #25)
> > > Created attachment 637026 [details] [diff] [review]
> > > part 1: remove flags
> >
> > I tried part 1 & 2, and now all I get is a neterror page because
> > system.gaiamobile.org can't be found. Which may just be a problem with using
> > an old database (since it's generated from an old xulrunner) with the new
> > queries. This also suggests this would fail to work after upgrades on
> > desktop.
>
> I have done some updates. Please ignore part 2, but apply part 1 and part
> 3. It seems work fine for me.
It works better than it did in comment 32. However, the performance benefit is not entirely satisfying. It cuts the wait time of the dialer test case in about a half, but that's still > 2s.
I'll attach storage logs for the template app and the dialer when placing a call.
Reporter | ||
Comment 38•13 years ago
|
||
This is from doing nothing on the main screen and tapping the template icon to the template application being displayed.
Reporter | ||
Comment 39•13 years ago
|
||
This is from having the dialer app opened, already having typed "555" and tapping the call button to the call information being displayed.
One interesting thing in both logs, which i was removing in my previous copy/paste because i was only grabbing logs according to their content, is that the first thing that happens is an update of the offline cache entry for something that was displayed and probably isn't relevant anymore.
That being said, there are still plenty of update and deletes, and in the dialer case, there's even a savepoint,
Comment 40•13 years ago
|
||
Thinker, please update the two patches that are ready to land right now and we will take the rest of the work to a new bug. Please do that first before anything else so we can get reviews for these in parallel to the additional follow-up work.
Updated•13 years ago
|
Attachment #637027 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #636918 -
Attachment is obsolete: true
Attachment #636918 -
Flags: review?(dcamp)
Updated•13 years ago
|
Attachment #637169 -
Flags: review?(dcamp)
Updated•13 years ago
|
Attachment #637170 -
Attachment description: part 3: track active entries in memory and refuse to delete them with raise(ignore) → part 2: track active entries in memory and refuse to delete them with raise(ignore)
Attachment #637170 -
Flags: review?(dcamp)
Comment 41•13 years ago
|
||
Actually patches look ready, I set the flag. Will clone the bug for follow-up work.
Updated•13 years ago
|
Attachment #637169 -
Flags: review?(dcamp) → review?(honzab.moz)
Updated•13 years ago
|
Attachment #637170 -
Flags: review?(dcamp) → review?(honzab.moz)
Updated•13 years ago
|
blocking-basecamp: --- → ?
![]() |
||
Comment 42•13 years ago
|
||
The patches are not buildable (clobber, up to date m-c):
mozStorageConnection.obj : error LNK2019: unresolved external symbol _sqlite3_result_error_code referenced in function "void __cdecl mozilla::storage::`anonym
ous namespace'::basicFunctionHelper(struct sqlite3_context *,int,struct Mem * *)" (?basicFunctionHelper@?A0xea6702a0@storage@mozilla@@YAXPAUsqlite3_context@@H
PAPAUMem@@@Z)
xul.dll : fatal error LNK1120: 1 unresolved externals
![]() |
||
Comment 43•13 years ago
|
||
Comment on attachment 637169 [details] [diff] [review]
part 1: remove flags
Review of attachment 637169 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ -1107,2 @@
> StatementSql ( mStatement_UpdateEntrySize, "UPDATE moz_cache SET DataSize = ? WHERE ClientID = ? AND Key = ?;" ),
> - StatementSql ( mStatement_UpdateEntryFlags, "UPDATE moz_cache SET Flags = ? WHERE ClientID = ? AND Key = ?;" ),
Remove also the member it self.
Attachment #637169 -
Flags: review?(honzab.moz) → review+
![]() |
||
Comment 44•13 years ago
|
||
Comment on attachment 637170 [details] [diff] [review]
part 2: track active entries in memory and refuse to delete them with raise(ignore)
Review of attachment 637170 [details] [diff] [review]:
-----------------------------------------------------------------
I don't see readdition of the code removed in part1@CreateCacheEntry.
Add a test the deletion prevention during eviction works. You can inspire with following tests: test_offlinecache_custom-directory.js and e.g. test_bug248970_cache.js. Best, create 3 entries, hold one of them and call the eviction API (both evict all and evict for clientid) and then check the files are still present (maybe just open input stream on the cache entry and try to read from it). You have to cover all places of the code you are changing. It's unfortunate we have just tests that may not test this deeply if at all (mochitests under dom/) what is not enough to catch potential regression by your changes.
Also r+ for part1 is conditioned by a good part2 (or 3?) followup patch. Both must be landed just together.
::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +1346,5 @@
> return nsnull;
> }
> +
> + // lock the entry
> + Lock(key);
Map the locking by the whole cache key (i.e. ClientID + key).
::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +257,5 @@
> nsCOMPtr<mozIStorageStatement> mStatement_FindClient;
> nsCOMPtr<mozIStorageStatement> mStatement_FindClientByNamespace;
> nsCOMPtr<mozIStorageStatement> mStatement_EnumerateGroups;
> nsCOMPtr<mozIStorageStatement> mStatement_EnumerateGroupsTimeOrder;
> + nsCOMPtr<mozIStorageStatement> mStatement_RaiseIgnore;
Unused.
::: storage/src/mozStorageConnection.cpp
@@ +213,5 @@
> }
> + int retcode = result ? variantToSQLiteT(aCtx, result) : SQLITE_OK;
> + if (retcode == SQLITE_IGNORE) {
> + ::sqlite3_result_error_code(aCtx, SQLITE_IGNORE);
> + } else if (retcode != SQLITE_OK) {
I'm not a peer for this code.
You need get this reviewed from someone else.
Attachment #637170 -
Flags: review?(honzab.moz) → review-
Assignee | ||
Comment 45•13 years ago
|
||
Attachment #637169 -
Attachment is obsolete: true
Attachment #637758 -
Flags: review+
Assignee | ||
Comment 46•13 years ago
|
||
Remove unused member and Lock() with clientID + key
Attachment #637170 -
Attachment is obsolete: true
Comment 47•13 years ago
|
||
Whats the status here? Can we get the reviews and land this?
Assignee | ||
Comment 48•13 years ago
|
||
A minor change with testcase proved.
Attachment #637759 -
Attachment is obsolete: true
Attachment #637838 -
Flags: review?
Assignee | ||
Comment 49•13 years ago
|
||
This is necessary to remove files in real for DOOM.
Attachment #637840 -
Flags: review?
Assignee | ||
Comment 50•13 years ago
|
||
make sure all files are removed as expected. Only inactive entry would be removed.
Attachment #637844 -
Flags: review?
Assignee | ||
Comment 51•13 years ago
|
||
(In reply to Andreas Gal :gal from comment #47)
> Whats the status here? Can we get the reviews and land this?
Let go~
![]() |
||
Comment 52•13 years ago
|
||
I'm taking a look quickly.
![]() |
||
Comment 53•13 years ago
|
||
Comment on attachment 637844 [details] [diff] [review]
Part 4: testcase
Review of attachment 637844 [details] [diff] [review]:
-----------------------------------------------------------------
Just an advice: the test can be hugely simplified. You can just write cache entries directly (open for write, take the out stream and write to it). You don't need the whole update mechanism to fill the offline cache. It actually just affects the test run time.
![]() |
||
Comment 54•13 years ago
|
||
Comment on attachment 637840 [details] [diff] [review]
Part 3: install eviction observer before deleting a entry
Review of attachment 637840 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +956,5 @@
> bool hasRows;
> rv = statement->ExecuteStep(&hasRows);
> NS_ENSURE_SUCCESS(rv, rv);
>
> + evictionObserver.Apply();
Doesn't DeleteData() call take care of the file deletion? I don't see other reason to add the eviction observer, since otherwise this all is a no-op.
Explain why exactly (in detail) why you have added this code here.
Attachment #637840 -
Flags: review? → review-
![]() |
||
Comment 55•13 years ago
|
||
Comment on attachment 637838 [details] [diff] [review]
part 2: track active entries in memory and refuse to delete them with raise(ignore), v3
Review of attachment 637838 [details] [diff] [review]:
-----------------------------------------------------------------
I'm still missing an alternative to the check for an entry being active @ CreateCacheEntry. Actually, FindEntry has to return nsnull immediately when the cache entry under the key is found active.
::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +108,3 @@
> " ON moz_cache FOR EACH ROW BEGIN SELECT"
> " cache_eviction_observer("
> + " OLD.ClientID || ':' || OLD.key, OLD.generation);"
Change the method's signature and pass both components separately.
@@ +191,5 @@
> + nsCAutoString fullKey;
> + rv = values->GetUTF8String(0, fullKey);
> + if (NS_FAILED(rv)) {
> + return nsnull;
> + }
Just curious: why can't you any more use AsSharedUTF8String?
@@ +1139,5 @@
> " AND NameSpace <= ?2 AND ?2 GLOB NameSpace || '*'"
> " ORDER BY NameSpace DESC;"),
> StatementSql ( mStatement_InsertNamespaceEntry, "INSERT INTO moz_cache_namespaces (ClientID, NameSpace, Data, ItemType) VALUES(?, ?, ?, ?);"),
> StatementSql ( mStatement_EnumerateGroups, "SELECT GroupID, ActiveClientID FROM moz_cache_groups;"),
> + StatementSql ( mStatement_EnumerateGroupsTimeOrder, "SELECT GroupID, ActiveClientID FROM moz_cache_groups ORDER BY ActivateTimeStamp;"),
I don't think you want that trailing colon.
@@ +1473,5 @@
> LOG(("nsOfflineCacheDevice::DoomEntry [key=%s]\n", entry->Key()->get()));
>
> + if (!entry->IsActive()) {
> + Unlock(entry->Key()->get());
> + }
Why is this needed? Why wasn't this needed before? Please add an explanation comment to the code.
Attachment #637838 -
Flags: review-
Assignee | ||
Comment 56•13 years ago
|
||
Comment on attachment 637840 [details] [diff] [review]
Part 3: install eviction observer before deleting a entry
Review of attachment 637840 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +956,5 @@
> bool hasRows;
> rv = statement->ExecuteStep(&hasRows);
> NS_ENSURE_SUCCESS(rv, rv);
>
> + evictionObserver.Apply();
For DoomEntry(), DeleteEntry() is called with deleteData==false. So, DeleteEntry() is never called for DoomEntry(). The testcase in part 4 show that nsCacheSession::DoomEntry() makes call nsOfflineCacheDevice::DoomEntry() with an inactive entry. That entry would never be deactivated, so the file is never be killed. So, I apply the observer here, to make the file be deleted only while it is not in activated.
By the way, I found there are easier ways to fix it. Just check if an entry is active, call DeleteEntry() with deleteData == false for active entry and deleteData == true for inactive entry. Another way is always activate entries at nsCacheService.
Assignee | ||
Comment 57•13 years ago
|
||
Change the way of handling DoomEntry for inactive entries.
Attachment #637838 -
Attachment is obsolete: true
Attachment #637840 -
Attachment is obsolete: true
Attachment #637838 -
Flags: review?
Attachment #637983 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 58•13 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #53)
> Comment on attachment 637844 [details] [diff] [review]
> Part 4: testcase
>
> Review of attachment 637844 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Just an advice: the test can be hugely simplified. You can just write cache
> entries directly (open for write, take the out stream and write to it). You
> don't need the whole update mechanism to fill the offline cache. It
> actually just affects the test run time.
I think it helps to figure interactive behavior between relative modules.
For example, with this testcase, I find nsCacheService will Doom an inactive entry.
![]() |
||
Comment 59•13 years ago
|
||
(In reply to Thinker Li [:sinker] from comment #58)
> I think it helps to figure interactive behavior between relative modules.
> For example, with this testcase, I find nsCacheService will Doom an inactive
> entry.
Actually, I'm against this argument. Your test should be as simple and narrowed to check only your code changes as possible. We have other tests (mochi) for interaction. I don't see a reason why to involve the offline cache update service here. But it is up to you to spent additional time on changing the test. This is not a review requirement.
Comment 60•13 years ago
|
||
Is the patch good to go minus the test?
![]() |
||
Comment 61•13 years ago
|
||
(In reply to Andreas Gal :gal from comment #60)
> Is the patch good to go minus the test?
No, I need to finish review of the part2 patch. Part1 cannot be landed w/o it, actually, I would like to land them merged as a single change set.
Since this is relatively urgent, the test is not needed to land with it, but I will watch for landing it ASAP after that.
Comment 62•13 years ago
|
||
Honza, how is that review coming along? We really need to make progress on this.
![]() |
||
Comment 63•13 years ago
|
||
Comment on attachment 637983 [details] [diff] [review]
Part 2: track active entries in memory and refuse to d elete by ignore
Review of attachment 637983 [details] [diff] [review]:
-----------------------------------------------------------------
r-
Emphasizing again: *removal of code from CreateCacheEntry in part1 patch has not been readded in part2 with the new implementation. FindEntry has to return null when there is an active entry under the demanded key.*
This may be either wrong (then please anybody comment with explanation) or simply has not been addressed.
::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +190,5 @@
> PRUint32 valueLen;
> + const char *clientID = values->AsSharedUTF8String(0, &valueLen);
> + const char *key = values->AsSharedUTF8String(1, &valueLen);
> + nsCAutoString fullKey(clientID);
> + fullKey.Append(key);
There is a colon (':') between clientID and the key. Thus, this cannot work at all. If you didn't catch this, then there is probably something wrong with the test.
@@ +1471,5 @@
>
> // We can go ahead and delete the corresponding row in our table,
> // but we must not delete the file on disk until we are deactivated.
> + // In another word, the file should be delete if the entry is
> + // deactivated.
"In another words, the file should be deleted only if the entry had been deactivated."
(Still may not be 100% correct English, though.)
::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +162,5 @@
> char ***keys);
>
> + bool IsLocked(const char *key);
> + void Lock(const char *key);
> + void Unlock(const char *key);
Let this take const nsACString&. You always have it in hands when working with this API and inside you just convert to nsDepenedentCString. I believe you are OK with it, see [1].
[1] http://hg.mozilla.org/mozilla-central/annotate/e0f64c714814/xpcom/glue/nsHashKeys.h#l142
::: storage/src/mozStorageConnection.cpp
@@ +213,5 @@
> }
> + int retcode = variantToSQLiteT(aCtx, result);
> + if (retcode == SQLITE_IGNORE) {
> + ::sqlite3_result_int(aCtx, SQLITE_IGNORE);
> + } else if (retcode != SQLITE_OK) {
Again, I cannot review this. Forwarding this piece to Shawn Wilsher.
Attachment #637983 -
Flags: review?(sdwilsh)
Attachment #637983 -
Flags: review?(honzab.moz)
Attachment #637983 -
Flags: review-
![]() |
||
Comment 64•13 years ago
|
||
Comment on attachment 637844 [details] [diff] [review]
Part 4: testcase
Review of attachment 637844 [details] [diff] [review]:
-----------------------------------------------------------------
r- based on that this test is not able to find a serious bug in the part2 patch.
Attachment #637844 -
Flags: review? → review-
Assignee | ||
Comment 65•13 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #63)
> Comment on attachment 637983 [details] [diff] [review]
> Part 2: track active entries in memory and refuse to d elete by ignore
>
> Review of attachment 637983 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r-
>
> Emphasizing again: *removal of code from CreateCacheEntry in part1 patch has
> not been readded in part2 with the new implementation. FindEntry has to
> return null when there is an active entry under the demanded key.*
I will check it at FindEntry or CreateCacheEntry.
>
> This may be either wrong (then please anybody comment with explanation) or
> simply has not been addressed.
>
> ::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
> @@ +190,5 @@
> > PRUint32 valueLen;
> > + const char *clientID = values->AsSharedUTF8String(0, &valueLen);
> > + const char *key = values->AsSharedUTF8String(1, &valueLen);
> > + nsCAutoString fullKey(clientID);
> > + fullKey.Append(key);
>
> There is a colon (':') between clientID and the key. Thus, this cannot work
> at all. If you didn't catch this, then there is probably something wrong
> with the test.
The colon between clientID and the key changes only the hash of the key and uniqueness of the key. I don' think it is a wrong of the testcase in part4. It just means we need another testcase for uniqueness of keys.
Assignee | ||
Comment 66•13 years ago
|
||
(In reply to Thinker Li [:sinker] from comment #65)
> (In reply to Honza Bambas (:mayhemer) from comment #63)
> > Comment on attachment 637983 [details] [diff] [review]
> > Part 2: track active entries in memory and refuse to d elete by ignore
> >
> > Review of attachment 637983 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > r-
> >
> > Emphasizing again: *removal of code from CreateCacheEntry in part1 patch has
> > not been readded in part2 with the new implementation. FindEntry has to
> > return null when there is an active entry under the demanded key.*
>
> I will check it at FindEntry or CreateCacheEntry.
>
> >
> > This may be either wrong (then please anybody comment with explanation) or
> > simply has not been addressed.
> >
> > ::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
> > @@ +190,5 @@
> > > PRUint32 valueLen;
> > > + const char *clientID = values->AsSharedUTF8String(0, &valueLen);
> > > + const char *key = values->AsSharedUTF8String(1, &valueLen);
> > > + nsCAutoString fullKey(clientID);
> > > + fullKey.Append(key);
> >
> > There is a colon (':') between clientID and the key. Thus, this cannot work
> > at all. If you didn't catch this, then there is probably something wrong
> > with the test.
>
> The colon between clientID and the key changes only the hash of the key and
> uniqueness of the key. I don' think it is a wrong of the testcase in part4.
> It just means we need another testcase for uniqueness of keys.
By the way, since the hash code is changed, I will fix it in the testcase.
Assignee | ||
Comment 67•13 years ago
|
||
Fix comment & format of Lock keys.
Attachment #637983 -
Attachment is obsolete: true
Attachment #637983 -
Flags: review?(sdwilsh)
Attachment #640558 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 68•13 years ago
|
||
Hold file "foo3" to keep it active during running evictEntries().
Attachment #637844 -
Attachment is obsolete: true
Attachment #640559 -
Flags: review?(sdwilsh)
Comment 69•13 years ago
|
||
sdwlish isn't around any more reliably. Give him a few hours to object, but in the meantime I will steal the review.
Comment 70•13 years ago
|
||
Comment on attachment 640558 [details] [diff] [review]
Part 2: track active entries in memory and refuse to d elete by ignore, v2
Review of attachment 640558 [details] [diff] [review]:
-----------------------------------------------------------------
Ok, lets get this in. App load performance is absolutely killing us right now.
::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +103,5 @@
> nsOfflineCacheEvictionFunction *evictionFunction)
> : mDB(db), mEvictionFunction(evictionFunction)
> {
> mDB->ExecuteSimpleSQL(
> + NS_LITERAL_CSTRING("CREATE TEMP TRIGGER cache_on_delete BEFORE DELETE"
Note that this slightly changes the failure mode. We can now crash before removing the row from the DB and at that point the file will be already gone. The lookup code property handles this case and ignores the entry, so its still safe. We should rip this code out soon and replace it with something sane instead. I have measured setting up and destroying the trigger taking 15ms ON MY DESKTOP. On the phone it must be an order of magnitude slower. Thinker has a new bug open to avoid these evictions on every fetch.
@@ +1498,5 @@
>
> // We can go ahead and delete the corresponding row in our table,
> // but we must not delete the file on disk until we are deactivated.
> + // In another word, the file should be deleted if the entry had been
> + // deactivated.
You mean "The file should be delete once the entry has been deactivated." ?
::: storage/src/mozStorageConnection.cpp
@@ +211,5 @@
> -1);
> return;
> }
> + int retcode = variantToSQLiteT(aCtx, result);
> + if (retcode == SQLITE_IGNORE) {
I verified that "SQLITE_IGNORE" isn't referenced in the code base before this patch (outside sqlite itself), so this change has zero risk.
Attachment #640558 -
Flags: review?(sdwilsh) → review+
Comment 71•13 years ago
|
||
Comment on attachment 640559 [details] [diff] [review]
Part 3: testcase, v2
I just hope we never change the hash format.
Attachment #640559 -
Flags: review?(sdwilsh) → review+
![]() |
||
Comment 72•13 years ago
|
||
Comment on attachment 640558 [details] [diff] [review]
Part 2: track active entries in memory and refuse to d elete by ignore, v2
Review of attachment 640558 [details] [diff] [review]:
-----------------------------------------------------------------
You could probably ask mak for a quick sanity-check on the storage/ bits.
r+'ing a patch that you originally wrote and that honza hasn't yet r+'d seems somewhat unorthodox...
Attachment #640558 -
Flags: feedback?(mak77)
Comment 73•13 years ago
|
||
Cool. Thanks for the referral.
Updated•13 years ago
|
Attachment #640558 -
Flags: feedback?(mak77) → review?(mak77)
![]() |
||
Comment 74•13 years ago
|
||
(In reply to Thinker Li [:sinker] from comment #65)
> (In reply to Honza Bambas (:mayhemer) from comment #63)
> > Comment on attachment 637983 [details] [diff] [review]
> > Part 2: track active entries in memory and refuse to d elete by ignore
> >
> > Review of attachment 637983 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > r-
> >
> > Emphasizing again: *removal of code from CreateCacheEntry in part1 patch has
> > not been readded in part2 with the new implementation. FindEntry has to
> > return null when there is an active entry under the demanded key.*
>
> I will check it at FindEntry or CreateCacheEntry.
So, any updates on this particular piece?
>
> >
> > This may be either wrong (then please anybody comment with explanation) or
> > simply has not been addressed.
> >
> > ::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
> > @@ +190,5 @@
> > > PRUint32 valueLen;
> > > + const char *clientID = values->AsSharedUTF8String(0, &valueLen);
> > > + const char *key = values->AsSharedUTF8String(1, &valueLen);
> > > + nsCAutoString fullKey(clientID);
> > > + fullKey.Append(key);
> >
> > There is a colon (':') between clientID and the key. Thus, this cannot work
> > at all. If you didn't catch this, then there is probably something wrong
> > with the test.
>
> The colon between clientID and the key changes only the hash of the key and
> uniqueness of the key. I don' think it is a wrong of the testcase in part4.
> It just means we need another testcase for uniqueness of keys.
I don't quit follow this argument, but maybe I'm missing something? The problem is that you were marking an entry in the hash table using <clientid>:<url>, but here you were just searching it only as <clientid><url>. So it could never be found in the hash table, right?
Assignee | ||
Comment 75•13 years ago
|
||
Fix the issue what Honza had mentioned in comment 63.
Attachment #640558 -
Attachment is obsolete: true
Attachment #640558 -
Flags: review?(mak77)
Attachment #640942 -
Flags: review?(mak77)
Attachment #640942 -
Flags: review?(gal)
Assignee | ||
Comment 76•13 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #74)
> I don't quit follow this argument, but maybe I'm missing something? The
> problem is that you were marking an entry in the hash table using
> <clientid>:<url>, but here you were just searching it only as
> <clientid><url>. So it could never be found in the hash table, right?
I didn't get the point at first instant. But, I catch your point and fix the issue at the v2.
Comment 77•13 years ago
|
||
Honza found the bug so ideally he should r+. I will steal if he is overloaded with review requests.
![]() |
||
Comment 78•13 years ago
|
||
Comment on attachment 640942 [details] [diff] [review]
Part 2: track active entries in memory and refuse to d elete by ignore, v3
Review of attachment 640942 [details] [diff] [review]:
-----------------------------------------------------------------
Thinker, please also address:
::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +162,5 @@
> char ***keys);
>
> + bool IsLocked(const char *key);
> + void Lock(const char *key);
> + void Unlock(const char *key);
Let this take const nsACString&. You always have it in hands when working with this API and inside you just convert to nsDepenedentCString. I believe you are OK with it, see [1].
[1] http://hg.mozilla.org/mozilla-central/annotate/e0f64c714814/xpcom/glue/nsHashKeys.h#l142
Assignee | ||
Comment 79•13 years ago
|
||
Change signatures of Lock(), Unlock() and IsLocked().
Attachment #640942 -
Attachment is obsolete: true
Attachment #640942 -
Flags: review?(mak77)
Attachment #640942 -
Flags: review?(gal)
Attachment #641414 -
Flags: review?(mak77)
Attachment #641414 -
Flags: review?(gal)
Assignee | ||
Updated•13 years ago
|
Attachment #641414 -
Flags: review?(gal) → review?(honzab.moz)
![]() |
||
Comment 80•13 years ago
|
||
Comment on attachment 641414 [details] [diff] [review]
Part 2: track active entries in memory and refuse to d elete by ignore, v4
Review of attachment 641414 [details] [diff] [review]:
-----------------------------------------------------------------
r=honzab for the network parts.
::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +190,5 @@
> PRUint32 valueLen;
> + const char *clientID = values->AsSharedUTF8String(0, &valueLen);
> + const char *key = values->AsSharedUTF8String(1, &valueLen);
> + nsCAutoString fullKey(clientID);
> + fullKey.AppendLiteral(":");
Append(NS_LITERAL_CSTRING(":")) might be a little bit faster here.
Attachment #641414 -
Flags: review?(honzab.moz) → review+
![]() |
||
Comment 81•13 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #80)
> ::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
> > + fullKey.AppendLiteral(":");
>
> Append(NS_LITERAL_CSTRING(":")) might be a little bit faster here.
AppendLiteral is actually going to be faster, as there's no runtime string length calculation. Less source code clutter, too.
![]() |
||
Comment 82•13 years ago
|
||
On IRC Nathan suggested to use even AppendLiteral(':') - taking just a char.
Comment 83•13 years ago
|
||
Any of these approaches works. A nano second doesn't matter here. Lets just land it.
Assignee | ||
Comment 84•13 years ago
|
||
(In reply to Andreas Gal :gal from comment #83)
> Any of these approaches works. A nano second doesn't matter here. Lets just
> land it.
Will mak77 review this?
Comment 85•13 years ago
|
||
Doesn't look like it. Storage is under-owned, and your change is trivial and I posted above an explanation why its extremely unlikely (or impossible) how that storage change will affect any other code in our code base, so r=me, and if anyone complains, I will take the fall (dear future complainer: please review, and post follow-up fix requests, we will take care of them).
Updated•13 years ago
|
Attachment #641414 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 86•13 years ago
|
||
r=honzab
Attachment #637758 -
Attachment is obsolete: true
Attachment #642486 -
Flags: review+
Assignee | ||
Comment 87•13 years ago
|
||
r=honzab,gal
Attachment #641414 -
Attachment is obsolete: true
Attachment #642487 -
Flags: review+
Assignee | ||
Comment 88•13 years ago
|
||
r=gal
Attachment #640559 -
Attachment is obsolete: true
Attachment #642488 -
Flags: review+
Comment 89•13 years ago
|
||
Try run for 3ddd552e89e8 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=3ddd552e89e8
Results (out of 215 total builds):
exception: 1
success: 194
warnings: 15
failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-3ddd552e89e8
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 90•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/61b4f7c9f526
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3c5bde390f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e69fbd397ce
Flags: in-testsuite+
Keywords: checkin-needed
Comment 91•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/61b4f7c9f526
https://hg.mozilla.org/mozilla-central/rev/9f3c5bde390f
https://hg.mozilla.org/mozilla-central/rev/4e69fbd397ce
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Updated•13 years ago
|
blocking-basecamp: ? → -
Comment 92•13 years ago
|
||
If this is a serious enough performance issue, please re-nom for blocking if this gets re-opened in the future.
You need to log in
before you can comment on or make changes to this bug.
Description
•