Using steps for adding visits seemed like a good idea at the time, but it has some major drawbacks: 1) Increased number of fsyncs, even if they don't happen on the main thread. 2) Lost atomicity of the work. Things can happen in the middle of our operations, which can lead to weird bugs. Due to (1), any main thread I/O that does happen can take much longer since the disk is being told to sync. This is especially true if the same file has that going on, which is the case with places. (2) just gives us more possibility for intermittent orange, and those things could happen to our users too.

I think I want to push this to try before I ask for review. Passes make check locally for places tests, which hit this code.

first-pass >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >--- a/toolkit/components/places/src/History.cpp >+++ b/toolkit/components/places/src/History.cpp >@@ -154,6 +154,7 @@ Step::HandleCompletion(PRUint16 aReason) > return NS_OK; > } > >+ wanted change? >+struct VisitData { >+ VisitData() >+ : placeId(0) >+ , visitId(0) any reason you init only these 2 and not also other uninitialized memory properties like sessionId, hidden, typed, transitionType, visitTime? >+ /** >+ * Copy constructor that is designed to not call AddRef/Release on the URI >+ * objects so they do not change their refcount on the wrong thread. >+ */ nit: comma after "objects"? >+class InsertVisitedURI : public nsRunnable >+{ >+public: >+ static nsresult Start(VisitData& aPlace, >+ nsIURI* aReferrer = NULL) is it generally ok to use NULL rather than nsnull? >+ { >+ NS_PRECONDITION(NS_IsMainThread(), "wrong thread"); I guess you want to use the same error message we use everywhere else >+ (void)history->GetDBConnection(getter_AddRefs(event->mDBConn)); this is scarying me, the connection could be in any state here and doesn't look like something is taking care of checking it. >+ // Speculatively get a new session id for our visit. >+ // TODO need to figure out story for new session IDs that isn't synchronous >+ // (bug 561450) >+ event->mPlace.sessionId = history->GetNewSessionID(); Actually I think we don't care much about the previous comment, GetNewSessionId runs a query for the first visit of a session, but any next visit will just get a cached increased value. Btw, this looks wrong, if the referrer has a sessionId we should not get a new one, but use its own. Even if previously or later we don't use the value, this would increase the sessionId without a reason. >+ NS_IMETHOD Run() >+ { >+ NS_PRECONDITION(!NS_IsMainThread(), "wrong thread"); ditto >+ // TODO transaction! I guess this has to be fixed :) >+ bool visited = GetPageInfo(mPlace); this looks a bit hybrid, I guess it should probably return a nsresult, and unvisited = rv == NS_ERROR_NOT_AVAILABLE, but if the method is small enough to swallow errors, it's ok >+ // The page was in moz_places, so we update nit: incomplete comment, we update what? >+ if (visited) { >+ NS_ASSERTION(mPlace.placeId != 0, "must have a valid place id!"); checking > 0 is even better >+ // The page was not in moz_places, so now we have to add it. >+ else { should probably move comments inside the if and the else body, or put a global comment before the if/else >+ nsCOMPtr<mozIStorageStatement> stmt; >+ nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >+ "INSERT INTO moz_places " >+ "(url, rev_host, hidden, typed, frecency) " >+ "VALUES (:page_url, :rev_host, :hidden, :typed, :frecency) " >+ ), getter_AddRefs(stmt)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ you could evaluate INSERT OR REPLACE at this point and have a unique code path, you have all the data for the visit in both cases if i'm not wrong. >+ // Now, we need to get the id of what we just added. >+ bool added = GetPageInfo(mPlace); what was visited is now added, looks a bit confusing even if it makes sense. >+ // If we had a referrer, we want to know about its last visit. specify "why" please >+ if (mReferrer.placeId != 0) { >+ bool recent = GetVisitInfo(mReferrer, mPlace.visitTime); >+ // At this point, we know the referrer's session id, which this new visit >+ // should also share. >+ if (recent) { >+ mPlace.sessionId = mReferrer.sessionId; >+ } >+ // However, if it isn't recent enough, we don't care to log anything about >+ // the referrer and we'll start a new session. >+ else { >+ mReferrer.visitId = 0; // This is sufficient to ignore our referrer nit: some comment is missing final period around. >+ >+ // Now we can add our visit... >+ nsresult rv = AddVisit(mPlace, mReferrer); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // ...and update the pages frecency. >+ rv = UpdateFrecency(mPlace); >+ NS_ENSURE_SUCCESS(rv, rv); the comment should probably say "page", not "pages" >+ bool GetPageInfo(VisitData& _place) >+ { >+ NS_PRECONDITION(_place.uri || _place.spec.Length(), >+ "must have a non-null uri or a non-empty spec!"); >+ if (_place.uri) { >+ rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), _place.uri); >+ } >+ else { >+ rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), _place.spec); >+ } >+ NS_ENSURE_SUCCESS(rv, false); looks like we set the spec when we know the uri, couldn't we fix later where you do aURI->GetSpec(place.spec) and just use .uri? >+ /** >+ * Loads visit information about the page into _place. >+ * >+ * @param _place >+ * The VisitData for the place we need to know visit information about. >+ * @param [optional] aThresholdStart >+ * The start time for the threshold to determine if the page was >+ * recently visited or not. This is optional but what happens if I don't specify it? >+ * @return true if the page was recently visited, false otherwise. Specify what does "recently" mean here >+ bool GetVisitInfo(VisitData& _place, >+ PRTime aThresholdStart = 0) >+ { >+ if (_place.uri) { >+ rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), _place.uri); >+ } >+ else { >+ rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), _place.spec); >+ } >+ NS_ENSURE_SUCCESS(rv, false); as above, could we just use uri? >+ // Only calculate if we are in the visit threshold if we were given a >+ // threshold start time. all these ifs are making me mad, could you invert the phrase please? >+ if (aThresholdStart && >+ aThresholdStart - _place.visitTime <= RECENT_EVENT_THRESHOLD) { >+ return true; >+ } I guess that aThresholdStart is always < _place.visitTime, thus you could invert the condition and avoid the first check on aThreasholdStart _place.visitTime - aThresholdStart < RECENT_EVENT_THRESHOLD, if aThresholdStart == 0 the condition is false but could be I'm confusing what thresholdStart is about, so please clarify the javadoc about what time it should be

(In reply to comment #2) > wanted change? negative > any reason you init only these 2 and not also other uninitialized memory > properties like sessionId, hidden, typed, transitionType, visitTime? fixed > is it generally ok to use NULL rather than nsnull? I think so, but I just made this use nsnull again. > this is scarying me, the connection could be in any state here and doesn't look > like something is taking care of checking it. Would you be happy with me making sure it's non-null? Anything else would just fail, but null could make us crash. > Actually I think we don't care much about the previous comment, GetNewSessionId > runs a query for the first visit of a session, but any next visit will just get > a cached increased value. As long as we do that during startup, I agree. > Btw, this looks wrong, if the referrer has a sessionId we should not get a new > one, but use its own. Even if previously or later we don't use the value, this > would increase the sessionId without a reason. We get the session id from the referrer if it's recent enough. I think consuming extra ones doesn't matter in reality. It's a 64-bit integer, so we have LOTS of space. > >+ // TODO transaction! > > I guess this has to be fixed :) Yeah, fixed it with an immediate transaction. > this looks a bit hybrid, I guess it should probably return a nsresult, and > unvisited = rv == NS_ERROR_NOT_AVAILABLE, but if the method is small enough to > swallow errors, it's ok This is all ran on the background thread anyway, and we can make assumptions upon failures just fine. I think not using nsresult is a bonus (we still console spew on failures). > nit: incomplete comment, we update what? Whoops. Fixed. > should probably move comments inside the if and the else body, or put a global > comment before the if/else I tweaked the comments to make it more clear. I'd prefer to keep them outside of the blocks. > you could evaluate INSERT OR REPLACE at this point and have a unique code path, > you have all the data for the visit in both cases if i'm not wrong. I will investigate this. > what was visited is now added, looks a bit confusing even if it makes sense. The added check is only for debugging anyway. I don't want to clobber the visited variable because we need it's value later. Would "visitWasAdded" work better for you? > >+ // ...and update the pages frecency. > >+ rv = UpdateFrecency(mPlace); > >+ NS_ENSURE_SUCCESS(rv, rv); > > the comment should probably say "page", not "pages" Actually, it needs to be "page's" > looks like we set the spec when we know the uri, couldn't we fix later where > you do aURI->GetSpec(place.spec) and just use .uri? I swear there was a reason for this at some point, but I can't figure out what it was anymore. It's possible the code has changed and it's no longer needed! > This is optional but what happens if I don't specify it? covered in the @return, which you've asked me to clarify.

(In reply to comment #3) > The added check is only for debugging anyway. I don't want to clobber the > visited variable because we need it's value later. Would "visitWasAdded" work > better for you? Actually, this is not correct. nsNavHistory::AddVisit cares about visitedness and notifies based on that, but this newer code just always notified (arguably better). However, it's useful to know if it was visited or not still so we don't make an extra call to GetPageInfo to get the id.

(In reply to comment #2) > you could evaluate INSERT OR REPLACE at this point and have a unique code path, > you have all the data for the visit in both cases if i'm not wrong. Cannot get this to work right. Things are coming up as not visited that are clearly visited as a result. Might have to do with frecency being reset in all cases now, but I had enough failures that I didn't think it was worth investigating any further.

Addresses comments. Need a part 2 that makes us cache the queries because I suspect that that is what hurts us right now on tp4 with this patch: http://tinyurl.com/23zster

Comment on attachment 484477 [details] [diff] [review] Part 1 - Use one event to do the work, and one to notify v1.0 >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >+ static nsresult Start(VisitData& aPlace, >+ nsIURI* aReferrer = nsnull) >+ { >+ nsNavHistory* history = nsNavHistory::GetHistoryService(); >+ NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY); >+ >+ nsRefPtr<InsertVisitedURI> event = new InsertVisitedURI(aPlace, aReferrer); >+ NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY); >+ >+ (void)history->GetDBConnection(getter_AddRefs(event->mDBConn)); get history before its first use >+ // Speculatively get a new session id for our visit. >+ // TODO need to figure out story for new session IDs that isn't synchronous >+ // (bug 561450) >+ event->mPlace.sessionId = history->GetNewSessionID(); I still don't like this. Even if we have plenty of numbers, a user can easily generate thousands of visits a day, creating large sessionId holes. Please don't set sessionId here. Later when adding to the db, if sessionId is 0, call GetNewSessionID to get a valid one (if it's not-zero a referrer did set it) So actually history getter would not even be needed here. There is a bit of confusion between history and navhistory. I guess if we should call the local var legacyHistory or navHistory when getting nsNavHistory::GetHistoryService() >+ // Get our target thread, and then start our work! nit: imho s/our/the/ >+ NS_IMETHOD Run() >+ { >+ NS_PRECONDITION(!NS_IsMainThread(), >+ "This should not be called on the main thread"); >+ >+ mozStorageTransaction transaction(mDBConn, PR_FALSE, >+ mozIStorageConnection::TRANSACTION_IMMEDIATE); >+ bool visited = GetPageInfo(mPlace); move getPageInfo before the transaction, there should be no gain in locking db for a select >+ // If the page was in moz_places, so we update the entry. messed up comment >+ if (visited) { the var name is still fighting with the comment, being in moz_places is not the same as being visited. maybe pageExists? >+ else { >+ NS_ASSERTION(mPlace.placeId == 0, "should not have a valid place id!"); >+ >+ nsCOMPtr<mozIStorageStatement> stmt; >+ // TODO investigate merging these two paths this todo should be addressed or moved before the if/else imo >+ rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("frecency"), -1); >+ NS_ENSURE_SUCCESS(rv, rv); Looks like we were wise, frecency has DEFAULT -1, so if you are adding a new entry, setting it is useless (in the INSERT OR REPLACE case it would not be though) >+ // Now, we need to get the id of what we just added. >+ bool added = GetPageInfo(mPlace); I guess if FetchPageInfo and FetchVisitInfo would be better names, the return param would be a generic "fetched" or "available", and we are actually fetching data and putting it into the passed in object. It really doesn't look like a classic value = GetSomething. >+ // Now we can add our visit... >+ nsresult rv = AddVisit(mPlace, mReferrer); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // ...and update the page's frecency. >+ rv = UpdateFrecency(mPlace); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // And commit our transaction. >+ rv = transaction.Commit(); >+ NS_ENSURE_SUCCESS(rv, rv); These comments are pretty much repeating the self commenting code. Can you get rid of them? >+ /** >+ * Loads information about the page into _place if the page is visited. Actually the comment is bogus, this fetches information if the page is in moz_places, without checking if it's visited. >+ * >+ * @param _place >+ * The VisitData for the place we need to know information about. >+ * @return true if the page was visited, false otherwise. ditto As I said above I think they should be FetchPageInfo and FetchVisitInfo and return value should be "true if the requested page/visit was found" >+ /** >+ * Loads visit information about the page into _place. >+ * >+ * @param _place >+ * The VisitData for the place we need to know visit information about. >+ * @param [optional] aThresholdStart >+ * The start time for the threshold to determine if the page was >+ * recently visited or not. "the start time" is still an unclear definition for me. What we subtract this time from? O is it an absolute time? Clarify definition please, I'd never know what to pass to this method without looking at the code. >+ nsresult AddVisit(VisitData& _place, >+ const VisitData& aReferrer) >+ { >+ nsCOMPtr<mozIStorageStatement> stmt; >+ nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >+ "INSERT INTO moz_historyvisits " >+ "(from_visit, place_id, visit_date, visit_type, session) " >+ "VALUES (:from_visit, :page_id, :visit_date, :visit_type, :session) " >+ ), getter_AddRefs(stmt)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("from_visit"), >+ aReferrer.visitId); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), >+ _place.placeId); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("visit_date"), >+ _place.visitTime); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("visit_type"), >+ _place.transitionType); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("session"), >+ _place.sessionId); I think this is the point where you want to call GetNewSessionId if sessionId == 0 I'd probably suggest to add some #ifdef DEBUG check that at this point all values are meaningful, won't matter for opt builds but would be cool to have (so all values > 0 but transitionType that should be one of the accepted values.). And the same where we add the page. >+ // Now that it should be in the database, we need to obtain the id of the >+ // visit we just added. >+ (void)GetVisitInfo(_place); I think could make sense to warn if this does not return true. globally looks good, I'd like to do a last pass once we have good Tp4 numbers though.

(In reply to comment #7) > I still don't like this. Even if we have plenty of numbers, a user can easily > generate thousands of visits a day, creating large sessionId holes. > Please don't set sessionId here. Later when adding to the db, if sessionId is > 0, call GetNewSessionID to get a valid one (if it's not-zero a referrer did set > it) I cannot do that because it's not on the main thread, which is why I am doing it here on the main thread. > Looks like we were wise, frecency has DEFAULT -1, so if you are adding a new > entry, setting it is useless (in the INSERT OR REPLACE case it would not be > though) Hmm, maybe I can get away with doing the INSERT OR REPLACE again if I don't pass the frecency value. Did we always set the default though, or was that a change we made later? > I'd probably suggest to add some #ifdef DEBUG check that at this point all > values are meaningful, won't matter for opt builds but would be cool to have > (so all values > 0 but transitionType that should be one of the accepted > values.). And the same where we add the page. Don't we have extensive test coverage on this though?

(In reply to comment #8) > > 0, call GetNewSessionID to get a valid one (if it's not-zero a referrer did set > > it) > I cannot do that because it's not on the main thread, which is why I am doing > it here on the main thread. that's sad :( please add a comment saying that. > > Looks like we were wise, frecency has DEFAULT -1, so if you are adding a new > > entry, setting it is useless (in the INSERT OR REPLACE case it would not be > > though) > Hmm, maybe I can get away with doing the INSERT OR REPLACE again if I don't > pass the frecency value. Did we always set the default though, or was that a > change we made later? Afaict, this went in from the first day of frecency > > I'd probably suggest to add some #ifdef DEBUG check that at this point all > > values are meaningful, won't matter for opt builds but would be cool to have > > (so all values > 0 but transitionType that should be one of the accepted > > values.). And the same where we add the page. > Don't we have extensive test coverage on this though? I'm unsure that the async addvisit test is checking each field, if so nevermind.

(In reply to comment #9) > that's sad :( please add a comment saying that. I mean, I could get fancy and try to make a way for us to return them, but I don't think that that is worthwhile.

To be clear, INSERT OR REPLACE won't work because SQLite will give us a new id, orphaning our moz_historyvisits entries from their moz_places entries.

This should address review comments. I'll work on part 2 tomorrow.

Comment on attachment 484907 [details] [diff] [review] Part 1 - Use one event to do the work, and one to notify v1.1 >+ * @param [optional] aThresholdStart >+ * The timestamp of a new visit (not represented by _place) used to >+ * determine if the page was recently visited or not. nit: timestamps are usually milliseconds, this is usecs PRTime.

Comment on attachment 484907 [details] [diff] [review] Part 1 - Use one event to do the work, and one to notify v1.1 >+ nsCOMPtr<mozIStorageStatement> stmt; >+ nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >+ "INSERT INTO moz_places " >+ "(url, rev_host, hidden, typed) " >+ "VALUES (:page_url, :rev_host, :hidden, :typed) " >+ ), getter_AddRefs(stmt)); >+ NS_ENSURE_SUCCESS(rv, rv); fwiw, with async steps I had found that in tests this was sometimes failing due to the fact another visit or favicon was trying to create the same entry, thus we used INSERT OR IGNORE for it (I don't recall the bug# offhand though). I guess with the immediate transaction that shouldn't happen but maybe we could do that as an additional safety

(In reply to comment #14) > I guess with the immediate transaction that shouldn't happen but maybe we could > do that as an additional safety Doesn't help us on the same database connection if we surrender the lock briefly (which we do!).

(In reply to comment #13) > nit: timestamps are usually milliseconds, this is usecs PRTime. Eh, that's news to me. What would you prefer I use here?

Decided to make part 2 just about the storage bits. This can easily be reused by all consumers if they want (although it does make them choose between sync and async, I think that that is OK). Part 3 will make us use them with the code in part 1.

Comment on attachment 485195 [details] [diff] [review] Part 2 - generic statement cache in storage v1.0 Should we be complaining if the cache is destroyed with things in its cache (implying the cache was not finalized)? Since I think the connection shutdown will complain if anything stayed alive and this stuff normally has to be strongly ordered, I'll say no.

Already changed my mind a bit about how this should work. I'd like to be able to just pass a reference to a COM ptr so that I don't have to have an existing database connection opened when I create this object.

(In reply to comment #18) > Should we be complaining if the cache is destroyed with things in its cache > (implying the cache was not finalized)? Since I think the connection shutdown > will complain if anything stayed alive and this stuff normally has to be > strongly ordered, I'll say no. Those were my thoughts as well.

(In reply to comment #19) > Created attachment 485197 [details] [diff] [review] > Part 2 - generic statement cache in storage v1.1 > > Already changed my mind a bit about how this should work. I'd like to be able > to just pass a reference to a COM ptr so that I don't have to have an existing > database connection opened when I create this object. I find this weird and scary. How can you enforce lifetime validity of the reference to the nsCOMPtr?

(the class member reference, not the function call signature that takes a reference)

(In reply to comment #21) > I find this weird and scary. How can you enforce lifetime validity of the > reference to the nsCOMPtr? I envision this to be used on the parent class, so: class History : public mozilla::storage::StatementCache<mozIStorageStatement> { public: History() : StatementCache<mozIStorageStatement>(mDBConn) { } private: ... nsCOMPtr<mozIStorageConnection> mDBConn; }; == or == class History { public: History() : mCache(StatementCache<mozIStorageStatement>(mDBConn)) { } private: ... mozilla::storage::StatementCache<mozIStorageStatement> mCache; nsCOMPtr<mozIStorageConnection> mDBConn; }; (note, the above code samples were not compiled). In these cases it's safe. I agree that in samples like the test cases it's a lot easier to shoot oneself in the foot, but we don't use storage that way anywhere in the tree AFAIK.

Comment on attachment 485197 [details] [diff] [review] Part 2 - generic statement cache in storage v1.1 Okay, that works, but please add a comment on the constructor or the template that explicitly calls out the life-cycle requirement. It doesn't need to be an essay, but since this is an abstraction layer and there are cases where a function/constructor can take an nsCOMPtr reference without actually saving off that reference later, I think it merits explicit mention.

Whoops - should have had a comment there in the first place. Added this locally: /** * Constructor for the cache. * * @note a connection can have more than one cache. * * @param aConnection * A reference to the nsCOMPtr for the connection this cache is to be * used for. This nsCOMPtr must at least live as long as this class, * otherwise crashes will happen. */

I also had to sprinkle the magical "inline" variable on every method to make the linker happy when it is included more than once (happens easily in libxul).

Updated based on comments.

Comment on attachment 485316 [details] [diff] [review] Part 3 - Hookup and use StatementCache >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >+ nsCOMPtr<mozIStorageStatement> stmt = >+ mHistory->syncStatementCache.GetCachedStatement( >+ "UPDATE moz_places " >+ "SET hidden = :hidden, typed = :typed " >+ "WHERE id = :page_id " >+ ); >+ NS_ENSURE_TRUE(stmt, NS_ERROR_UNEXPECTED); NS_ENSURE_STATE? >+ nsCOMPtr<mozIStorageStatement> stmt = >+ mHistory->syncStatementCache.GetCachedStatement( >+ "INSERT INTO moz_places " >+ "(url, rev_host, hidden, typed) " >+ "VALUES (:page_url, :rev_host, :hidden, :typed) " >+ ); >+ NS_ENSURE_TRUE(stmt, NS_ERROR_UNEXPECTED); ditto >@@ -604,16 +613,17 @@ private: >- nsCOMPtr<mozIStorageStatement> stmt; >- nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >- "INSERT INTO moz_historyvisits " >- "(from_visit, place_id, visit_date, visit_type, session) " >- "VALUES (:from_visit, :page_id, :visit_date, :visit_type, :session) " >- ), getter_AddRefs(stmt)); >- NS_ENSURE_SUCCESS(rv, rv); >+ nsCOMPtr<mozIStorageStatement> stmt = >+ mHistory->syncStatementCache.GetCachedStatement( >+ "INSERT INTO moz_historyvisits " >+ "(from_visit, place_id, visit_date, visit_type, session) " >+ "VALUES (:from_visit, :page_id, :visit_date, :visit_type, :session) " >+ ); >+ NS_ENSURE_TRUE(stmt, false); looks like this should return a nsresult? (NS_ENSURE_STATE) >@@ -651,43 +661,54 @@ private: > nsresult UpdateFrecency(const VisitData& aPlace) > { >- // First, set our frecency to the proper value. >- nsCOMPtr<mozIStorageStatement> stmt; >- nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >- "UPDATE moz_places " >- "SET frecency = CALCULATE_FRECENCY(:page_id) " >- "WHERE id = :page_id" >- ), getter_AddRefs(stmt)); >- NS_ENSURE_SUCCESS(rv, rv); >+ { // First, set our frecency to the proper value. >+ nsCOMPtr<mozIStorageStatement> stmt = >+ mHistory->syncStatementCache.GetCachedStatement( >+ "UPDATE moz_places " >+ "SET frecency = CALCULATE_FRECENCY(:page_id) " >+ "WHERE id = :page_id" >+ ); >+ NS_ENSURE_TRUE(stmt, false); nsresult here too >+ { // Now, we need to mark the page as not hidden if the frecency is now >+ // nonzero. >+ nsCOMPtr<mozIStorageStatement> stmt = >+ mHistory->syncStatementCache.GetCachedStatement( >+ "UPDATE moz_places " >+ "SET hidden = 0 " >+ "WHERE id = :page_id AND frecency <> 0" >+ ); >+ NS_ENSURE_TRUE(stmt, false); and here >+ >+ /** >+ * Strong reference to the History object because we do not want it to >+ * disappear out from under us. >+ */ >+ nsRefPtr<History> mHistory; As said on IRC ownership of History seems to be messed up or under-evaluated :( Not part of this bug though. >@@ -868,6 +889,7 @@ History* History::gService = NULL; > > History::History() > : mShuttingDown(false) >+, syncStatementCache(mDBConn) uh wait, who is setting mDBConn here? >+nsresult >+History::EnsureDatabaseConnectionExists() >+{ >+ if (mDBConn) { >+ return NS_OK; >+ } nit: I prefer the if(!mDBConn) bracing here with single exit point >+ >+ nsNavHistory* history = nsNavHistory::GetHistoryService(); >+ NS_ENSURE_TRUE(history, NS_ERROR_UNEXPECTED); NS_ENSURE_STATE >+ nsresult rv = history->GetDBConnection(getter_AddRefs(mDBConn)); >+ NS_ENSURE_SUCCESS(rv, rv); as well as you could avoid rv and NS_ENSURE_STATE(mDBConn) >-NS_IMPL_ISUPPORTS2( >+NS_IMPL_THREADSAFE_ADDREF(History) >+NS_IMPL_THREADSAFE_RELEASE(History) >+NS_IMPL_QUERY_INTERFACE2( I thought we had NS_IMPL_THREADSAFE_ISUPPORTS2, how does it differ? >diff --git a/toolkit/components/places/src/History.h b/toolkit/components/places/src/History.h >+ /** >+ * Statement cache that is used for background thread statements only. >+ */ >+ storage::StatementCache<mozIStorageStatement> syncStatementCache; As I said on IRC, it's unclear if this is sync-StatementCache or syncStatement-Cache. I'd suggest to drop Cache and just call it syncStatements (plural), if anybody has doubt of what is it, the header is there to check (not that Cache is saying anything on the real type). > private: > virtual ~History(); > > /** >+ * Checks to make sure that we have a valid database connection (via mDBConn). >+ */ >+ nsresult EnsureDatabaseConnectionExists(); this method is also caching the connection, so Exists is somehow reducing its real scope (indeed I'd expect it to return a bool). Why not making it GetDatabaseConnection() and NS_ENSURE_STATE its result? >+ /** >+ * A read-write database connection used for adding history visits and setting >+ * a page's title. >+ * >+ * @note this is the same connection as the one found on nsNavHistory. >+ */ >+ nsCOMPtr<mozIStorageConnection> mDBConn; ...and if going for GetDBConnection add a note about always using the getter. Otherwise it needs a note to always call EnsureDB before using it)

(In reply to comment #29) > >@@ -868,6 +889,7 @@ History* History::gService = NULL; > > > > History::History() > > : mShuttingDown(false) > >+, syncStatementCache(mDBConn) > > uh wait, who is setting mDBConn here? it's a reference to our nsCOMPtr, so it can be updated later. This is why I call EnsureDatabaseConnection before using mDBConn. > nit: I prefer the if(!mDBConn) bracing here with single exit point However, that goes against the style guide and how we've done this in other places in the file already. > I thought we had NS_IMPL_THREADSAFE_ISUPPORTS2, how does it differ? Indeed we do. Must be newish.

Comment on attachment 485407 [details] [diff] [review] Part 3 - Hookup and use StatementCache v1.1 >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp > History::History() > : mShuttingDown(false) >+, syncStatements(mDBConn) Just as a clarity step, please add a comment regarding the face mDBConn is lazy inited later and to check the javadoc on it. >@@ -977,11 +1001,7 @@ History::GetIsVisitedStatement() >+mozIStorageConnection* >+History::GetDBConn() >+{ >+ if (mDBConn) { >+ return mDBConn; >+ } ... >+ >+ return mDBConn; >+} Could even be code style, but looks pretty bad. FWIW dietrich always told me to prefer single exit point for short methods and early return for long ones, and I agree (indeed Places style mostly followed that). btw, this is really NIT! >diff --git a/toolkit/components/places/src/History.h b/toolkit/components/places/src/History.h >+ /** >+ * Statement cache that is used for background thread statements only. >+ */ >+ storage::StatementCache<mozIStorageStatement> syncStatements; So, if these are for background thread how do we define the async ones? for async background thread? looks fine now!

Additionally, let's remove the set title steps too. Also gets rid of a bunch of other support code (and adds a few assertions to things that should have had them in part 2).

Improved documentation and assertions, and fixes an issue where we were not releasing the URI on the main thread.

Comment on attachment 485927 [details] [diff] [review] Part 4 - No steps for setting the page title v1.1 >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >-#ifdef MOZ_IPC >-#include "mozilla/dom/ContentChild.h" >-#include "nsXULAppAPI.h" >-#endif ah cool, this looks like coming from a double merge :( >@@ -74,87 +69,6 @@ namespace places { >+class NotifyTitleObservers : public nsRunnable > { > public: >+ NotifyTitleObservers(bool aNotify, >+ nsCOMPtr<nsIURI>& aURI, >+ const nsString& aTitle) reasoning behind nsString rather than nsAString? >+ : mNotify(aNotify) >+ , mTitle(aTitle) > { >+ NS_PRECONDITION(!NS_IsMainThread(), >+ "This should not be called on the main thread"); >+ >+ // Do not want to AddRef and Release on the backgroundthread! missing space in comment >+ // First, see if the page exists in the database (we'll need its id later). > nsCOMPtr<mozIStorageStatement> stmt = >+ mHistory->syncStatements.GetCachedStatement( >+ "SELECT id, title " >+ "FROM moz_places " >+ "WHERE url = :page_url " >+ ); > NS_ENSURE_STATE(stmt); > >+ PRInt64 placeId = 0; >+ nsString title; nsAutoString here? >+ { >+ mozStorageStatementScoper scoper(stmt); >+ nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mURI); >+ NS_ENSURE_SUCCESS(rv, rv); > >+ PRBool hasResult; >+ rv = stmt->ExecuteStep(&hasResult); >+ NS_ENSURE_SUCCESS(rv, rv); >+ if (!hasResult) { >+ // We have no record of this page, so there is no need to do any further >+ // work. >+ return Finish(false); So, if I understand this correctly, passing aNotify false is mostly a no-op, it is needed only to release mURI on the main thread. Who is freeing mURI if any of these calls fail before callinf Finish()? is not there risk that it is still freed on the async thread? It's possible you need a macro that replaces NS_ENSURE_SUCCESS calling Finish(false) before returning >+private: >+ SetPageTitle(mozIStorageConnection* aConnection, >+ nsIURI* aURI, >+ const nsString& aTitle) >+ : mDBConn(aConnection) >+ , mURI(aURI) >+ , mTitle(aTitle) >+ , mHistory(History::GetService()) >+ { >+ } add a check for mHistory validity, like an assertion? >-void > History::Shutdown() > { > mShuttingDown = true; It's hard to figure out final status of the file with all these changes, do we still use mShuttingDown anywhere? Should we? most likely we'd fail to execute anything on the connection (binding would fail). Could save some warning failure on shutdown mostly. >@@ -1325,19 +1228,19 @@ History::SetURITitle(nsIURI* aURI, const > return NS_OK; > } > >- nsAutoPtr<SetTitleData> data(new SetTitleData()); >- NS_ENSURE_STATE(data); >- >- data->uri = aURI; >- >+ nsString title; Why not a nsAutoString? r+ with these addressed/answered

Part 1 v1.1 had a minor bug in it which caused some test failures. This fixes that.

(In reply to comment #35) > reasoning behind nsString rather than nsAString? It's better to use the concrete type when it is available. > >+ PRInt64 placeId = 0; > >+ nsString title; > > nsAutoString here? Yes, I was just being silly I think. > So, if I understand this correctly, passing aNotify false is mostly a no-op, it > is needed only to release mURI on the main thread. > Who is freeing mURI if any of these calls fail before callinf Finish()? is not > there risk that it is still freed on the async thread? > It's possible you need a macro that replaces NS_ENSURE_SUCCESS calling > Finish(false) before returning I don't think it's warranted. There are two failure cases here: 1) we fail to allocate a NotifyTitleObservers object. I don't actually even need that check (and I've removed them from this file now) since we have infallible new now. 2) we fail dispatching to the main thread. Looking through that code, it will only fail if...we run out of memory (which we don't have to worry about). > add a check for mHistory validity, like an assertion? I'm not sure it's worthwhile - the only way we get created is by that very class. We also don't check it anywhere else. > It's hard to figure out final status of the file with all these changes, do we > still use mShuttingDown anywhere? Yes, all the public facing API (declared in IHistory) checks it and returns early if we are shutting down. > >+ nsString title; > > Why not a nsAutoString? I was being silly again.

per comments

There was a subtle bug with this statement cache that only linux seemed to catch (and only with my last push to try oddly enough). I need to pass N - 1 to the nsDependentCString constructor and not N though, and this patch reflects that.

Eww. Try server found a fun thread safety bug for me: xul!nsCOMPtr<nsIURI>::~nsCOMPtr<nsIURI>+0x000000000000003B (e:\builds\moz2_slave\tryserver-win32-debug\build\obj-firefox\dist\include\nscomptr.h, line 534) xul!mozilla::places::`anonymous namespace'::NotifyTitleObservers::~NotifyTitleObservers+0x000000000000001D xul!mozilla::places::`anonymous namespace'::NotifyTitleObservers::`scalar deleting destructor'+0x000000000000000F xul!nsRunnable::Release+0x000000000000008F (e:\builds\moz2_slave\tryserver-win32-debug\build\obj-firefox\xpcom\build\nsthreadutils.cpp, line 55) xul!nsCOMPtr<nsIRunnable>::~nsCOMPtr<nsIRunnable>+0x000000000000003B (e:\builds\moz2_slave\tryserver-win32-debug\build\obj-firefox\dist\include\nscomptr.h, line 534) xul!mozilla::places::`anonymous namespace'::SetPageTitle::Finish+0x000000000000010F (e:\builds\moz2_slave\tryserver-win32-debug\build\toolkit\components\places\src\history.cpp, line 823) xul!mozilla::places::`anonymous namespace'::SetPageTitle::Run+0x0000000000000993 (e:\builds\moz2_slave\tryserver-win32-debug\build\toolkit\components\places\src\history.cpp, line 788) xul!nsThread::ProcessNextEvent+0x00000000000001FA (e:\builds\moz2_slave\tryserver-win32-debug\build\xpcom\threads\nsthread.cpp, line 547) xul!NS_ProcessNextEvent_P+0x0000000000000053 (e:\builds\moz2_slave\tryserver-win32-debug\build\obj-firefox\xpcom\build\nsthreadutils.cpp, line 250) xul!nsThread::ThreadFunc+0x00000000000000CE (e:\builds\moz2_slave\tryserver-win32-debug\build\xpcom\threads\nsthread.cpp, line 263) nspr4!_PR_NativeRunThread+0x00000000000000DB (e:\builds\moz2_slave\tryserver-win32-debug\build\nsprpub\pr\src\threads\combined\pruthr.c, line 426) nspr4!pr_root+0x0000000000000019 (e:\builds\moz2_slave\tryserver-win32-debug\build\nsprpub\pr\src\md\windows\w95thred.c, line 122) MSVCR80D!beginthreadex+0x0000000000000221 MSVCR80D!beginthreadex+0x00000000000001C7 kernel32!GetModuleHandleA+0x00000000000000DF ###!!! ASSERTION: nsStandardURL not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file e:/builds/moz2_slave/tryserver-win32-debug/build/netwerk/base/src/nsStandardURL.cpp, line 929 ###!!! ASSERTION: nsScriptCacheCleaner not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file e:/builds/moz2_slave/tryserver-win32-debug/build/content/base/src/nsFrameMessageManager.cpp, line 723 What happened here is that in Finish, some time after we call NS_DispatchToMainThread and before we exit Finish, the event runs on the main thread. It will not free the object since we just free the object in the destructor of the event, which in this case still has a refcount of 1 because the background thread is still alive. I can fix this by explicitly Releasing the URI on the main thread.

Fixes our release issue in all three places we could hit it.

looks like you attached the same patch as part 1 and part 4

Looks like I uploaded the wrong part 1. Will fix when I get into the office.

The real part 1

My code changes the timing of the observer topic this tests uses. Blair, can you review the test change only please?

Looks like you did not address comment 14? I think we can't ensure forever ordering of who will add the page when multiple visits and favicons are being added, using OR IGNORE on inserting a new page should be safe and better.

(In reply to comment #46) > Looks like you did not address comment 14? > I think we can't ensure forever ordering of who will add the page when multiple > visits and favicons are being added, using OR IGNORE on inserting a new page > should be safe and better. Sorry, I thought I responded to that. That only really was a problem when we queued things up, but we don't do that anymore with the patches in this bug. The call to AddVisit is going to happen before we can even parse if a page has a favicon.

I see, using the same thread they can't interrupt each other. It looks like still an issue if the main thread adds the same page synchronously (this could happen in b-c tests using addVisit or other sync APIs while visiting pages) while your work is ongoing in the background thread.

(In reply to comment #48) > I see, using the same thread they can't interrupt each other. > It looks like still an issue if the main thread adds the same page > synchronously (this could happen in b-c tests using addVisit or other sync APIs > while visiting pages) while your work is ongoing in the background thread. True, but I'd argue that that is a buggy test at that point and we should fix it.

http://hg.mozilla.org/projects/places/rev/f9a3c390c537 http://hg.mozilla.org/projects/places/rev/7d3efce2d46d http://hg.mozilla.org/projects/places/rev/3b477671678a http://hg.mozilla.org/projects/places/rev/47cdf340a182 http://hg.mozilla.org/projects/places/rev/797ce3f26f04

We should block on this because it substantially decreases the number of writes, reads, and fsyncs that we do (measured during tp4): Read: 25,254 (702,259,820 bytes, 361,212 us) -77 (.3%), -2,501,610 bytes (.35%), -2,960 us (.81%) Write: 12,737 (307,897,717 bytes, 1,652,908 us) -3,625 (22.2%), -83,631,025 bytes (21.4%), -1,155,970 us (41.2%) Flush: 951 (24,973,623 us) -559 (37.0%), -15,413,900 us (38.2%)

Seeing some orange on the Places tree due to nsJSURI not having an implementation of GetHostname, so GetReversedHostname fails, which causes us to return early on the background thread, which causes the URI to be released on the wrong thread. Fun times. This just makes it so we don't add javascript: URIs to history (via nsINavHistoryService::canAddURI), which seemed like the right approach anyway. One test was depending on this, which is also fixed in the patch (by making it a bookmark, which we still support).

Comment on attachment 489216 [details] [diff] [review] Part 6 - do not add javascript: URIs to history r+ provided you also update toolkit/components/places/tests/unit/test_isvisited.js to check for javascript: uris I suggest you to do a tryrun on linux debug, just to be sure that there isn't some docshell or places mochitest that depends on waiting visited status of a javascript uri. There should not be, but a simple linux test should take few time and resources.

Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=c8f82d60b4c0

One test failed on try, which this fixes.

And because I suspect we have no issues (and he said r+ on irc): http://hg.mozilla.org/projects/places/rev/025fdaee74b6

(In reply to comment #53) > r+ provided you also update > toolkit/components/places/tests/unit/test_isvisited.js to check for javascript: > uris Whoops - forgot to do this. Will push a follow-up tomorrow.

(In reply to comment #57) > Whoops - forgot to do this. Will push a follow-up tomorrow. Or today: http://hg.mozilla.org/projects/places/rev/c3025efd8542

Plus a follow-up to get rid of some orange: http://hg.mozilla.org/projects/places/rev/4626f19fa27e

Still seeing assertions about nsStandardUrl not being threadsafe on tinderbox on the places branch. I've now gone and audited the code, and made sure we always end up releasing on the main thread only (with NS_ProxyRelease). For extra safety, I went ahead and cloned the URI too before handing it off to the background thread just to make sure that nobody else might be touching it on the main thread while we use it on the background thread. Pushed to the try server at http://tbpl.mozilla.org/?tree=MozillaTry&rev=20f160755c37

Comment on attachment 489672 [details] [diff] [review] Part 7 - be super safe with nsIURI objects No good. Will try a different approach tomorrow.

Not yet [fixed-in-places] since I still have threadsafety issues with the URI objects. Bug 575188 will let me see what I'm missing in the stacks, so adding a dependency.

(In reply to comment #62) > Not yet [fixed-in-places] since I still have threadsafety issues with the URI > objects. Bug 575188 will let me see what I'm missing in the stacks, so adding > a dependency. Actually, spun the threadsafety stuff out into bug 612617.

http://hg.mozilla.org/mozilla-central/rev/025fdaee74b6 http://hg.mozilla.org/mozilla-central/rev/47cdf340a182 http://hg.mozilla.org/mozilla-central/rev/3b477671678a http://hg.mozilla.org/mozilla-central/rev/c3025efd8542 http://hg.mozilla.org/mozilla-central/rev/4626f19fa27e http://hg.mozilla.org/mozilla-central/rev/7d3efce2d46d http://hg.mozilla.org/mozilla-central/rev/f9a3c390c537