Closed Bug 1310295 Opened 9 years ago Closed 7 years ago

Make left pane queries virtual

Categories

(Toolkit :: Places, defect, P1)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Project Flags:
Performance Impact low
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: lina, Assigned: standard8)

References

(Blocks 6 open bugs)

Details

(Keywords: perf)

Attachments

(2 files)

Bug 1310295 - Make left pane queries virtual in the Places Library window.
59 bytes, text/x-review-board-request
lina
: review+
mak
: review+
Details
Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots.
59 bytes, text/x-review-board-request
mak
: review+
Details
These queries are currently created by http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/browser/components/places/PlacesUIUtils.jsm#1138-1156, and stored as children of the Places root. In bug 1258127, comment 190, Mak suggests making them virtual, or hardcoding the list of queries: > The left pane folder is something that could probably be converted to a > "virtual" folder. By this I mean we could have something like > RESULTS_AS_TAG_QUERY, but it being RESULTS_AS_LEFTPANE_FOLDER that returns a > fake result. Or we could hardcode the Library left pane contents, it's a > long time we want to move the library to an in-content UI, and the current > tree is not the best visual for that. That may allow us to have no real left > pane folder.
Priority: -- → P3
Blocks: 1161091
Priority: P3 → P2
See Also: → PlacesInTabLibrary
Marco is this related to the code that powers the primary bookmarking UI? If it is than it should be a qf:p1. Please let us know. Thanks.
Flags: needinfo?(mak77)
Whiteboard: [qf] → [qf:p1]
This is needed to be able to remove the old synchronous bookmarking API, as most of the other blockers of bug 519514. The code is used by the Library left pane. As a bonus, it would also go towards allowing us to simplify some of the Sync code, making synchronization cheaper.
Flags: needinfo?(mak77)
If this improves the performance of the Library window which isn't part of our primary UI we should probably bump down the priority to P3...
Whiteboard: [qf:p1] → [qf:p3]
Keywords: perf
Blocks: 1410877
Blocks: 1083465
I've started looking at this.
Assignee: nobody → standard8
Status: NEW → ASSIGNED
I suggest to start by replacing allBookmarksFolderId, that is a subquery and should be simpler. Then the whole left pane folder will return this new query among the others.
Depends on: 1423896
Blocks: 824502
Priority: P2 → P1
I've just attached the current WIP. The basic view works in the left-pane, however the test failures show there's various selection issues that I need to resolve yet. I suspect there's a few other bits to fix up yet, including the XXX comments I've left in there.
Blocks: 1441695
Blocks: 1441697
Blocks: 1441698
No longer blocks: 1083465
Blocks: 1437516
I think the patch is now functionally complete for the left pane. The only thing left to do is to add a migration routine for the database to remove any non-default root items. I'm letting it run on try overnight, and I'll do some more manual testing tomorrow, however, any comments welcome in the meantime.
Having discussed with Marco on IRC, we've been through the options of using database migration, UI migration & idle places maintenance. The maintenance isn't really an option because this doesn't guarantee to get us to the position where "everything in the db can be synced". The UI migration isn't viable as it doesn't re-migrate if the user downgrades and then upgrades. This leaves us with database migration, which is possible but more complex than the other two options, as we can't use the already written bookmark APIs already available to us. In Marco's words, this is what we need to do: temp trigger to update foreign_count, recursive depth-first CTE query to delete all the bookmarks, remove orphan places and annos... Kit, is it ok to delete these without Sync noticing, or do we need to do more work?
Flags: needinfo?(kit)
(In reply to Mark Banner (:standard8) from comment #11) > Kit, is it ok to delete these without Sync noticing, or do we need to do > more work? This sounds like a good plan! I think the temp trigger should also insert the removed GUIDs into `moz_bookmarks_deleted`, so that we upload tombstones for them. Since bug 1274496, Sync ignores items that aren't under the four user content roots. However, for older accounts, some or all of the left pane items might have been accidentally uploaded and synced to other devices, causing issues like bug 1436897. Other devices will ignore tombstones for items that they don't know about, so this is OK to do unconditionally.
Flags: needinfo?(kit)
Status update: I think the main patch here is ready to start the review cycle. I'm still working on the migration patch (only test written so far), but I think it is separate enough that we can start reviews progressing on the main changes.
Comment on attachment 8956125 [details] Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots. I've created a test for the migration that I think should cover everything we need to check post-migration. If you could take a look that'll be good. In the meantime, I'll start thinking about writing the actual migration routine.
Attachment #8956125 - Flags: feedback?(mak77)
Comment on attachment 8954107 [details] Bug 1310295 - Make left pane queries virtual in the Places Library window. https://reviewboard.mozilla.org/r/223252/#review231624 a discussed over IRC, we need a blocker bug preventing "insert" and "update" adding stuff into the root ::: toolkit/components/places/nsNavHistoryResult.cpp:2122 (Diff revision 3) > nsNavHistoryResult* result = GetResult(); > NS_ENSURE_STATE(result); > > - if (mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY || > - mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_UNIFIED) { > + if ((mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY || > + mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_UNIFIED) && > + mLiveUpdate != QUERYUPDATE_NONE) { what about instead we bailout early with if (mLiveUpdate == QUERYUPDATE_NONE) { mContentsValid = true; return NS_OK; }
Attachment #8954107 - Flags: review?(mak77) → review+
Depends on: 1443835
Comment on attachment 8956125 [details] Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots. looks good so far
Attachment #8956125 - Flags: feedback?(mak77) → feedback+
Note: some of the test removals from the first patch have now moved to bug 1443835. I'm intending to land these at the nearly same time and it made sense to do that, rather than fix up the tests.
Comment on attachment 8954107 [details] Bug 1310295 - Make left pane queries virtual in the Places Library window. https://reviewboard.mozilla.org/r/223252/#review232280 Awesome!
Attachment #8954107 - Flags: review?(kit) → review+
Comment on attachment 8956125 [details] Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots. https://reviewboard.mozilla.org/r/225056/#review232618 Unsolicited drive-by SQL comments. :-) ::: toolkit/components/places/Database.cpp:2006 (Diff revision 3) > + > + // This trigger listens for moz_places deletes, and updates moz_annos and > + // moz_keywords accordingly. > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "CREATE TEMP TRIGGER moz_migrate_annos_trigger " > + "AFTER DELETE ON moz_places FOR EACH ROW " Triggers cascade, so you could also make this an `AFTER UPDATE` trigger `WHEN NEW.foreign_count = 0 AND NEW.visit_count = 0`, and take out the `DELETE FROM moz_places` above. ::: toolkit/components/places/Database.cpp:2032 (Diff revision 3) > + " WHERE id = OLD.place_id; " > + "END " > + )); > + if (NS_FAILED(rv)) return rv; > + > + // First of all, find the non-builtin roots. If you wanted to, I think you could do the deletions in a single statement, and mop up non-folder children of the Places root with something like: ``` WITH RECURSIVE itemsToRemove(id, guid) AS ( SELECT b.id, b.guid FROM moz_bookmarks b WHERE b.guid = 'root________' UNION ALL SELECT b.id, b.guid FROM moz_bookmarks b JOIN itemsToRemove d ON d.id = b.parent WHERE b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______') ) DELETE FROM itemsToRemove WHERE guid <> 'root________' ``` If you intentionally broke out `DeleteBookmarkFolderContents`, or Mak thinks differently, please ignore me. :-) ::: toolkit/components/places/Database.cpp:2093 (Diff revision 3) > + )); > + if (NS_FAILED(rv)) return rv; > + > + // Cleanup any orphan annotation attributes. > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DELETE FROM moz_anno_attributes WHERE id IN ( " Nit: I think you can remove the subselect on `moz_anno_attributes` and do `DELETE FROM moz_anno_attributes WHERE NOT EXISTS (SELECT a.id FROM moz_annos a WHERE a.anno_attribute_id = id) ...`.
Comment on attachment 8956125 [details] Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots. https://reviewboard.mozilla.org/r/225056/#review232618 > Nit: I think you can remove the subselect on `moz_anno_attributes` and do `DELETE FROM moz_anno_attributes WHERE NOT EXISTS (SELECT a.id FROM moz_annos a WHERE a.anno_attribute_id = id) ...`. Sorry, I'm totally wrong. >.<
Comment on attachment 8956125 [details] Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots. https://reviewboard.mozilla.org/r/225056/#review232770 ::: toolkit/components/places/Database.cpp:1985 (Diff revision 3) > + "CREATE TEMP TRIGGER moz_migrate_bookmarks_trigger " > + "AFTER DELETE ON moz_bookmarks FOR EACH ROW " > + "BEGIN " > + // Insert tombstones. > + " INSERT INTO moz_bookmarks_deleted (guid, dateRemoved) " > + " VALUES (OLD.guid, %lld); " nit: fwiw, Sqlite has time functions, for example yo ucan get a timestamp with SELECT strftime('%s', 'now', 'localtime', 'utc') * 1000 ::: toolkit/components/places/Database.cpp:2032 (Diff revision 3) > + " WHERE id = OLD.place_id; " > + "END " > + )); > + if (NS_FAILED(rv)) return rv; > + > + // First of all, find the non-builtin roots. Kit suggestion to do the delete one should could be more performante. I'd also like if we'd first run a select to check if there's anything to remove, and in case there isn't bailout even before creating the triggers. I want to limit work for profiles that don't have anything to delete (the majority) So I'd say to run a select to see if there's any extraneous root, if there isn't any just bailout. Otherwise, add the triggers and do the one shot removal. ::: toolkit/components/places/Database.cpp:2058 (Diff revision 3) > + > + int32_t type; > + rv = stmt->GetInt32(2, &type); > + if (NS_FAILED(rv)) return rv; > + > + printf("id: %d, guid: %s, type: %d\n", id, guid.get(), type); debug leftover ::: toolkit/components/places/Database.cpp:2077 (Diff revision 3) > + " foreign_count = (SELECT COUNT(*) FROM moz_keywords WHERE place_id = p.id) " > + ")" > + )); > + if (NS_FAILED(rv)) return rv; > + > + // Now remove the temp trigger. nit: triggers (plural) ::: toolkit/components/places/Database.cpp:2099 (Diff revision 3) > + " SELECT id from moz_anno_attributes n " > + " WHERE NOT EXISTS " > + " (SELECT id FROM moz_annos WHERE anno_attribute_id = n.id LIMIT 1) " > + " AND NOT EXISTS " > + " (SELECT id FROM moz_items_annos WHERE anno_attribute_id = n.id LIMIT 1) " > + ")" Probably you could use EXCEPT in a more efficient way. (also uppercase FROM, per Places SQL coding style). Please double check it does the right thing, usually selects group from left to right: SELECT id FROM moz_anno_attributes EXCEPT SELECT DISTINCT anno_attribute_id FROM moz_annos EXCEPT SELECT DISTINCT anno_attribute_id FROM moz_items_annos ::: toolkit/components/places/tests/migration/test_current_from_v43.js:45 (Diff revision 3) > + Assert.equal(PlacesUtils.history.databaseStatus, > + PlacesUtils.history.DATABASE_STATUS_UPGRADED); > + > + let db = await PlacesUtils.promiseDBConnection(); > + Assert.equal((await db.getSchemaVersion()), CURRENT_SCHEMA_VERSION); > +}); MAybe we should open a connection to v43 and sanity check the initial status? do we trust the tombstone check and review process enough to prevent people messing with the v43 file?
Attachment #8956125 - Flags: review?(mak77)
Comment on attachment 8956125 [details] Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots. https://reviewboard.mozilla.org/r/225056/#review233414 ::: toolkit/components/places/Database.cpp:1967 (Diff revision 5) > + "SELECT b.id, b.guid FROM moz_bookmarks b " > + "JOIN itemsToRemove d ON d.id = b.parent " > + "WHERE b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______') " > + ") " > + "SELECT * FROM itemsToRemove " > + "WHERE guid <> 'root________' LIMIT 1 " We don't need to be recursive here, you could just: SELECT id FROM moz_bookmarks b JOIN moz_bookmarks p ON b.parent = p.id WHERE p.guid = "root________" AND b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______') You should also put this stmt into a scope, so it's finalized immediately on scope exist. Anyway, as you told me on IRC the likelihood that there's nothing to remove is very low, since at the minimum there is the left pane folder, so I was probably overzealous about this early bailout. If you want to remove it completely I won't complain. ::: toolkit/components/places/Database.cpp:1987 (Diff revision 5) > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "CREATE TEMP TRIGGER moz_migrate_bookmarks_trigger " > + "AFTER DELETE ON moz_bookmarks FOR EACH ROW " > + "BEGIN " > + // Insert tombstones. > + "INSERT INTO moz_bookmarks_deleted (guid, dateRemoved) " Just in case, I'd use INSERT OR IGNORE here, since guid is a primary key in moz_bookmarks_deleted ::: toolkit/components/places/Database.cpp:1988 (Diff revision 5) > + "CREATE TEMP TRIGGER moz_migrate_bookmarks_trigger " > + "AFTER DELETE ON moz_bookmarks FOR EACH ROW " > + "BEGIN " > + // Insert tombstones. > + "INSERT INTO moz_bookmarks_deleted (guid, dateRemoved) " > + "VALUES (OLD.guid, (SELECT strftime('%s', 'now', 'localtime', 'utc') * 1000)); " you can avoid the sub select and just insert strftime() as a value, for example INSERT INTO test VALUES (NULL, strftime('%s', 'now', 'localtime', 'utc') * 1000) ::: toolkit/components/places/Database.cpp:2008 (Diff revision 5) > + "CREATE TEMP TRIGGER moz_migrate_annos_trigger " > + "AFTER UPDATE ON moz_places FOR EACH ROW " > + // Only remove from moz_places if we don't have any remaining keywords pointing to > + // this place, and it hasn't been visited. Note: orphan keywords are tidied up below. > + "WHEN NEW.foreign_count = (SELECT COUNT(*) FROM moz_keywords WHERE place_id = NEW.id) AND " > + "NEW.visit_count = 0 " let's put the visit_count check first... ideally the optimizer should already do that, but hinting it is better ::: toolkit/components/places/Database.cpp:2050 (Diff revision 5) > + "SELECT b.id, b.guid FROM moz_bookmarks b " > + "JOIN itemsToRemove d ON d.id = b.parent " > + "WHERE b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______') " > + ") " > + "DELETE FROM moz_bookmarks " > + "WHERE guid <> 'root________' AND id IN (SELECT id FROM itemsToRemove) " I think you can rewrite the CTE as WITH RECURSIVE itemsToRemove(id, guid) AS ( SELECT b.id, b.guid FROM moz_bookmarks b JOIN moz_bookmarks p ON b.parent = p.id WHERE p.guid = 'root________' AND b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______') UNION ALL SELECT b.id, b.guid FROM moz_bookmarks b JOIN itemsToRemove d ON d.id = b.parent ) And then avoid the guid <> root condition outside, the CTE will already select all the unexpected children of root, and their children ::: toolkit/components/places/Database.cpp:2064 (Diff revision 5) > + // so that we can make use of the keyword trigger to update the counts in > + // moz_places. > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DELETE FROM moz_keywords WHERE place_id IN ( " > + "SELECT id FROM moz_places p WHERE " > + "foreign_count != 0 AND " please use <> instead of !=, the latter is not a SQL standard ::: toolkit/components/places/Database.cpp:2066 (Diff revision 5) > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DELETE FROM moz_keywords WHERE place_id IN ( " > + "SELECT id FROM moz_places p WHERE " > + "foreign_count != 0 AND " > + "foreign_count = (SELECT COUNT(*) FROM moz_keywords WHERE place_id = p.id) " > + ")" For the internal select, I think this may be a bit faster, because there is no index on foreign_count, and your query is selecting based on its value, that ends up doing a SCAN TABLE on moz_places SELECT h.id FROM moz_keywords k JOIN moz_places h ON h.id = k.place_id GROUP BY place_id HAVING h.foreign_count = count(*) ::: toolkit/components/places/Database.cpp:2091 (Diff revision 5) > + "DELETE FROM moz_anno_attributes WHERE id IN ( " > + "SELECT id FROM moz_anno_attributes n " > + "EXCEPT " > + "SELECT DISTINCT anno_attribute_id FROM moz_annos " > + "EXCEPT " > + "SELECT DISTINCT anno_attribute_id FROM moz_items_annos " nit: usually we don't indent for UNION,INTERSECT,EXCEPT. So just: SELECT EXCEPT SELECT EXCEPT SELECT
Attachment #8956125 - Flags: review?(mak77) → review+
Comment on attachment 8956125 [details] Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots. https://reviewboard.mozilla.org/r/225056/#review233414 > We don't need to be recursive here, you could just: > SELECT id > FROM moz_bookmarks b > JOIN moz_bookmarks p ON b.parent = p.id > WHERE p.guid = "root________" > AND b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______') > > You should also put this stmt into a scope, so it's finalized immediately on scope exist. > > Anyway, as you told me on IRC the likelihood that there's nothing to remove is very low, since at the minimum there is the left pane folder, so I was probably overzealous about this early bailout. If you want to remove it completely I won't complain. I just checked with creating a blank profile in FF 58.0.2 and creating one bookmark - the left-pane items do get created. Hence I'm going to drop this check.
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b1af75c617ea Make left pane queries virtual in the Places Library window. r=kitcambridge,mak https://hg.mozilla.org/integration/autoland/rev/a277657bfffa Provide a Places database migration routine to remove non-built-in roots. r=mak
We're working on the mochitest failures. The TV failures are tier-2, and appear to be already existing (xref bug 1436175).
Flags: needinfo?(standard8)
Try results with the fixed patch look good, so pushing again.
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eb12946f24c7 Make left pane queries virtual in the Places Library window. r=kitcambridge,mak https://hg.mozilla.org/integration/autoland/rev/4f96e58f2ece Provide a Places database migration routine to remove non-built-in roots. r=mak
Blocks: 1310299
https://hg.mozilla.org/mozilla-central/rev/eb12946f24c7 https://hg.mozilla.org/mozilla-central/rev/4f96e58f2ece
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1446672
Depends on: 1472127
Performance Impact: --- → P3
Whiteboard: [qf:p3]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: