Closed
Bug 727275
Opened 12 years ago
Closed 9 years ago
Remove 1 second from the main UI thread for a download from Download Manager
Categories
(Toolkit :: Downloads API, defect)
Tracking
()
RESOLVED
INVALID
mozilla13
People
(Reporter: bbondy, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
7.67 KB,
patch
|
Details | Diff | Splinter Review |
This is a profiling snapshot taken when downloading winrar. The download itself only takes a few seconds. The format of the below is X.YYY:ZZ YYY is the number of milliseconds, X would be seconds. nsDownload::SetState # of calls: 4 Total time: 0.998:94 Average time: 0.249:73 If you split apart this function you have the following breakdown: nsDownload::SetState::1 # of calls: 4 Total time: 0.014:07 Average time: 0.003:51 nsDownload::SetState::2 # of calls: 4 Total time: 0.959:17 Average time: 0.239:79 nsDownload::SetState::3 # of calls: 4 Total time: 0.025:52 Average time: 0.006:38 nsDownload::SetState::4 # of calls: 4 Total time: 0.000:00 Average time: 0.000:0:0 > nsresult > nsDownload::SetState(DownloadState aState) > { > PROFILE_FUNC(); > > NS_ASSERTION(mDownloadState != aState, > "Trying to set the download state to what it already is set to!"); > > PRInt16 oldState = mDownloadState; > mDownloadState = aState; > > // We don't want to lose access to our member variables > nsRefPtr<nsDownload> kungFuDeathGrip = this; > > { > PROFILE_STR("nsDownload::SetState::1"); > // When the state changed listener is dispatched, queries to the database and > // the download manager api should reflect what the nsIDownload object would > // return. So, if a download is done (finished, canceled, etc.), it should > // first be removed from the current downloads. We will also have to update > // the database *before* notifying listeners. At this point, you can safely > // dispatch to the observers as well. > switch (aState) { > ... > ... > ... > default: > break; > } > } > > // Before notifying the listener, we must update the database so that calls > // to it work out properly. > nsresult rv; > { > PROFILE_STR("nsDownload::SetState::2"); > rv = UpdateDB(); > NS_ENSURE_SUCCESS(rv, rv); > } > > { > PROFILE_STR("nsDownload::SetState::3"); > > mDownloadManager->NotifyListenersOnDownloadStateChange(oldState, this); > } > > { > PROFILE_STR("nsDownload::SetState::4"); > > switch (mDownloadState) { > ... > ... > ... > default: > break; > } > } > return NS_OK; >} I also profiled into UpdateDB and all of the time is spent in: > instmt->Execute();
Reporter | ||
Comment 1•12 years ago
|
||
Including mak's review comment from the other bug: Comment on attachment 596496 [details] [diff] [review] [diff] [review] Patch v1. Review of attachment 596496 [details] [diff] [review] [diff] [review]: ----------------------------------------------------------------- (In reply to Brian Smith (:bsmith) from comment #13) > (In reply to Brian R. Bondy [:bbondy] from comment #12) > > So the biggest bottleneck on the main thread happens in nsDownload::SetState. > > After each scan it does about 150ms-200ms on the main thread. Most the > > problem in Setstate is with the update to the DB: Hm, that time doesn't make any sense, considered that it's a simple UPDATE statement on primary key, that surely may cause a slower fsync, but not enough to take that much time. As far as I can tell nobody ever reported a slow query in the DM, and it would be pretty much visible in about:telemetry. Are you sure it's the statement and not rather the notification? May be the statement is slow just because the disk is kept extremely busy by the antivirus check? can we measure this? Btw, one issue with this change it's that it's introducing thread contention issues into the download manager, that by itself means solving an hang issue to introduce one potentially worse. Bug 699854 is filed to convert all downloads backend to async Storage to avoid contentions. To clarify, once you start using both synchronous and asynchronous statements on the same connection, you must be aware they share a common mutex, and they can race for it. That may be or not a problem, depending on the database traffic you expect at that time. Are there other synchronous writes on the database during this period apart updateState ones? ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +1944,5 @@ > return CancelDownload(id); > } else if (strcmp(aTopic, "profile-before-change") == 0) { > mGetIdsForURIStatement->Finalize(); > mUpdateDownloadStatement->Finalize(); > + mozilla::DebugOnly<nsresult> rv = mDBConn->AsyncClose(NULL); nsnull please @@ +3091,5 @@ > + // Just copy-construct ourselves except for mRefCnt. > + // The state of mDownloadState may change after the DB call, but we want to > + // notify the observers about the state at this instant. > + nsDownload *dl = new nsDownload(*this); > + if (!dl) { new is infallible afaik @@ +3094,5 @@ > + nsDownload *dl = new nsDownload(*this); > + if (!dl) { > + return NS_ERROR_OUT_OF_MEMORY; > + } > + dl->mRefCnt = 0; Could you rather make a proper copy constructor for nsDownload rather than changing the RefCnt here? Or could we just pass the current mDownloadState to the callback in addition and use that later? I don't like that much touching the RefCnt in code like this. @@ +3099,5 @@ > + > + nsRefPtr<DownloadStateDBListener> callback = > + new DownloadStateDBListener(dl, aOldState); > + nsCOMPtr<mozIStoragePendingStatement> handle; > + return stmt->ExecuteAsync(callback, getter_AddRefs(handle)); please assign to rv, check it and return NS_OK per code convention in this module ::: toolkit/components/downloads/nsDownloadManager.h @@ +465,5 @@ > + { > + } > + > + NS_DECL_ISUPPORTS > + NS_IMETHOD HandleCompletion(unsigned short aReason) add newline after NS_DECL_ISUPPORTS @@ +467,5 @@ > + > + NS_DECL_ISUPPORTS > + NS_IMETHOD HandleCompletion(unsigned short aReason) > + { > + NS_ENSURE_ARG_POINTER(mDownload); How could this fail? (In reply to Brian R. Bondy [:bbondy] from comment #19) > > + > > + NS_DECL_ISUPPORTS > > + NS_IMETHOD HandleCompletion(unsigned short aReason) > > + { > > + NS_ENSURE_ARG_POINTER(mDownload); > > It can't currently. This was added as a protection against future code > changes. please use MOZ_ASSERT for these kind of protections
Reporter | ||
Updated•11 years ago
|
Assignee: netzen → nobody
Reporter | ||
Comment 2•9 years ago
|
||
This probably isn't valid anymore.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•