Closed
Bug 605463
Opened 14 years ago
Closed 14 years ago
Lazily initialize history statements
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla2.0b9
People
(Reporter: mak, Assigned: mak)
References
Details
(Keywords: perf, Whiteboard: [Ts][fixed-in-places])
Attachments
(2 files)
(deleted),
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sdwilsh
:
approval2.0+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
This is on top of Places branch
Attachment #484307 -
Flags: review?(sdwilsh)
Comment 2•14 years ago
|
||
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•14 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•14 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 5•14 years ago
|
||
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•14 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)
Comment 7•14 years ago
|
||
(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•14 years ago
|
Attachment #484307 -
Flags: approval2.0+
Assignee | ||
Comment 8•14 years ago
|
||
Updated•14 years ago
|
Attachment #484906 -
Flags: approval2.0+
Updated•14 years ago
|
Attachment #484307 -
Flags: approval2.0+
Assignee | ||
Comment 9•14 years ago
|
||
Whiteboard: [Ts] → [Ts][fixed-in-places]
Assignee | ||
Comment 10•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/68616dfb2bc9
this is sorta tested by existing tests
Status: ASSIGNED → RESOLVED
Closed: 14 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.
Description
•