All users were logged out of Bugzilla on October 13th, 2018

Lazily initialize history statements

RESOLVED FIXED in mozilla2.0b9

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({perf})

Trunk
mozilla2.0b9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Ts][fixed-in-places])

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
we don't need all the statements immediately, and this can also help (in future) cleaning up a bunch of other code that uses statements that could be cached.
(Assignee)

Comment 1

8 years ago
Created attachment 484307 [details] [diff] [review]
patch v1.0

This is on top of Places branch
Attachment #484307 - Flags: review?(sdwilsh)
Note, we may want to consider copying what indexeddb code does:
http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBTransaction.cpp#495

This is easier than tracking which member variable we need, I think.
(Assignee)

Comment 3

8 years ago
that looks like a longer work, I'd prefer doing it for 4.1, since it should be done for other services too and it's requiring to copy statements all around.
(Assignee)

Comment 4

8 years ago
also, our method allows to have most of the statements in one point, rather than scattering them around. if we use the query as a key, we end up having to fix it across all the board. doesn't look like a good moment to do that now that we just rewrote queries and some of them could still need tweaking.
Comment on attachment 484307 [details] [diff] [review]
patch v1.0

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>+  // These statements are served through GetStatementByStoragePool.  Since the
>+  // method is const they can't be lazy inited by it.  Thus init them now.
>+  (void*)GetStatement(mDBPageInfoForFrecency);
>+  (void*)GetStatement(mDBAsyncThreadPageInfoForFrecency);
>+  (void*)GetStatement(mDBVisitsForFrecency);
>+  (void*)GetStatement(mDBAsyncThreadVisitsForFrecency);
Why do we need to even init them now?  Can't we just do that at first use?

r=sdwilsh
Attachment #484307 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 6

8 years ago
I added the comment above for that reason, if they are lazy inited on first use the method that inits them can't be const. but it has to be const because it's called through a const history object (frecency is on the async thread)
(In reply to comment #6)
> I added the comment above for that reason, if they are lazy inited on first use
> the method that inits them can't be const. but it has to be const because it's
> called through a const history object (frecency is on the async thread)
Oh, that's what that meant.  The comment was not clear to me; could you try to rephrase please?

Updated

8 years ago
Attachment #484307 - Flags: approval2.0+
(Assignee)

Comment 8

8 years ago
Created attachment 484906 [details] [diff] [review]
patch v1.1

Updated

8 years ago
Attachment #484906 - Flags: approval2.0+

Updated

8 years ago
Attachment #484307 - Flags: approval2.0+
(Assignee)

Comment 9

8 years ago
http://hg.mozilla.org/projects/places/rev/68616dfb2bc9
Whiteboard: [Ts] → [Ts][fixed-in-places]
(Assignee)

Updated

8 years ago
Depends on: 606053
(Assignee)

Updated

8 years ago
Blocks: 606157
(Assignee)

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/68616dfb2bc9

this is sorta tested by existing tests
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.