500306 - lazy-init annotation service statements

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Dietrich Ayala (:dietrich)]

hm, i thought i filed a bug for this, but can't find it.

idea is that many anno statements aren't needed in the startup path. so this patch just lazy-inits all of them.

needs some cleanup still, but asking quick review on the approach.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: wip

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 384985 [details] [diff] [review]
wip

we could follow the lazy approach, but would be cool to find a way to make it less verbose, having to remember to call an init function before using any statement sounds error-prone. I would prefer a temp stmt = GetAnnoStatement(mDBXXX), or we could inherit an annoStatementScoper from mozIStatementScoper that ensures the query is inited and then starts scoping as usual.
As a first step you should check if aStatement is already defined, no need to init a statement twice, then the structure should be an if-elseif or switch, rather than an if-if.
Btw, i have some concern about the functionality of this code, you are comparing aStatement with mDBxxx properties, but the latter are not yet defined, all of them are pointing to 0. Physical addressed of the COMPtrs are different though. I'm not sure what kind of comparison will be done in this case, the only plausible one would be direct address comparison (&mDBXXX is unique). If this works with an else-if structure it's already doing an address comparison, otherwise all ifs are probably returning true at the first call since aStatement points to 0 as all the mDBXXX.

btw, atm my repository is completely broken and i can't verify that.

Comment 3

[Marco Bonardo [:mak]]

i verified the above, both point to 0, so all ifs are returning true this way

Comment 4

[Marco Bonardo [:mak]]

stealing bugs around :p

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This one should work.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Fix possible shutdown bad conditions.

If you like this approach we could use it in other services too, replacing the single GetXXXStatement methods.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 417477 [details] [diff] [review]
patch v1.1


>+mozIStorageStatement*
>+nsAnnotationService::GetStatement(const nsCOMPtr<mozIStorageStatement>& aStmt)
>+{
>+#define RETURN_IF_STMT(_stmt, _sql)                                        \
>+  PR_BEGIN_MACRO                                                           \
>+  if (address_of(_stmt) == address_of(aStmt)) {                            \
>+    if (!_stmt) {                                                          \
>+      nsresult rv = mDBConn->CreateStatement(_sql, getter_AddRefs(_stmt)); \
>+      NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && _stmt, nsnull);                   \
>+    }                                                                      \
>+    return _stmt.get();                                                    \
>+  }                                                                        \
>+  PR_END_MACRO
>+
>+  if (mShuttingDown)
>+    return nsnull;

could put this before the macro, yes?

r=me otherwise

Comment 8

[Marco Bonardo [:mak]]

sure but would not change anything, the macro is expanded by the compiler

Comment 9

[Marco Bonardo [:mak]]

and, as a personal preference, i prefer having macros before code

Comment 10

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/58359ad70d1b