Closed Bug 605463 Opened 10 years ago Closed 10 years ago

Lazily initialize history statements

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b9

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: perf, Whiteboard: [Ts][fixed-in-places])

Attachments

(2 files)

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.
Attached patch patch v1.0Splinter Review
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.
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.
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+
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?
Attachment #484307 - Flags: approval2.0+
Attached patch patch v1.1Splinter Review
Attachment #484906 - Flags: approval2.0+
Attachment #484307 - Flags: approval2.0+
http://hg.mozilla.org/projects/places/rev/68616dfb2bc9
Whiteboard: [Ts] → [Ts][fixed-in-places]
Depends on: 606053
Blocks: 606157
http://hg.mozilla.org/mozilla-central/rev/68616dfb2bc9

this is sorta tested by existing tests
Status: ASSIGNED → RESOLVED
Closed: 10 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.