Closed Bug 1423612 Opened 2 years ago Closed 2 years ago

coverity complains about passing VisitData by value instead of by reference

Categories

(Toolkit :: Places, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: froydnj, Assigned: dthayer)

References

Details

Attachments

(1 file)

The following inefficiencies showed up in a Coverity scan today:

** CID 1425896:  Performance inefficiencies  (PASS_BY_VALUE)
/toolkit/components/places/History.cpp: 646 in mozilla::places::<unnamed>::NotifyManyVisitsObservers::NotifyVisit(nsNavHistory *, nsCOMPtr<nsIObserverService> &, long, mozilla::places::VisitData)()


________________________________________________________________________________________________________
*** CID 1425896:  Performance inefficiencies  (PASS_BY_VALUE)
/toolkit/components/places/History.cpp: 646 in mozilla::places::<unnamed>::NotifyManyVisitsObservers::NotifyVisit(nsNavHistory *, nsCOMPtr<nsIObserverService> &, long, mozilla::places::VisitData)()
640         aPlaces.SwapElements(mPlaces);
641       }
642
643       nsresult NotifyVisit(nsNavHistory* aNavHistory,
644                            nsCOMPtr<nsIObserverService>& aObsService,
645                            PRTime aNow,
>>>     CID 1425896:  Performance inefficiencies  (PASS_BY_VALUE)
>>>     Passing parameter aPlace of type "mozilla::places::VisitData" (size 160 bytes) by value.
646                            VisitData aPlace) {
647         nsCOMPtr<nsIURI> uri;
648         MOZ_ALWAYS_SUCCEEDS(NS_NewURI(getter_AddRefs(uri), aPlace.spec));
649         if (!uri) {
650           return NS_ERROR_UNEXPECTED;
651         }

This one is particularly bad, since we call NotifyVisit in a loop in NotifyManyVisitsObservers::Run.

** CID 1425899:  Performance inefficiencies  (PASS_BY_VALUE)
/toolkit/components/places/History.cpp: 629 in mozilla::places::<unnamed>::NotifyManyVisitsObservers::NotifyManyVisitsObservers(mozilla::places::VisitData)()


________________________________________________________________________________________________________
*** CID 1425899:  Performance inefficiencies  (PASS_BY_VALUE)
/toolkit/components/places/History.cpp: 629 in mozilla::places::<unnamed>::NotifyManyVisitsObservers::NotifyManyVisitsObservers(mozilla::places::VisitData)()
623     /**
624      * Notifies observers about a visit or an array of visits.
625      */
626     class NotifyManyVisitsObservers : public Runnable
627     {
628     public:
>>>     CID 1425899:  Performance inefficiencies  (PASS_BY_VALUE)
>>>     Passing parameter aPlace of type "mozilla::places::VisitData" (size 160 bytes) by value.
629       explicit NotifyManyVisitsObservers(VisitData aPlace)
630         : Runnable("places::NotifyManyVisitsObservers")
631         , mPlace(aPlace)
632         , mHistory(History::GetService())
633       {
634       }

It looks like these are a result of bug 1418443, but it also looks like there are several followup bugs that may change this code in the relatively near future.

Doug, can you make these pass-by-reference parameters?
Flags: needinfo?(dothayer)
Woops! Yeah of course.
Flags: needinfo?(dothayer)
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Is it possible to fix the NotifyManyVisitsObserver constructor, too?  Preferably by Move()'ing the VisitData in, but taking it by `const&` would be better than what we have now.
Changed to a const reference. Moving it is doable, it just strikes me as a little bit excessive, since we have other runnables that we instantiate using VisitDatas which co-occur with this, so deciding which should do a copy and which should do a move and ensuring they happen in the right order seems like a nasty business. Open to rebuttals though.
(In reply to Doug Thayer [:dthayer] from comment #5)
> Changed to a const reference. Moving it is doable, it just strikes me as a
> little bit excessive, since we have other runnables that we instantiate
> using VisitDatas which co-occur with this, so deciding which should do a
> copy and which should do a move and ensuring they happen in the right order
> seems like a nasty business. Open to rebuttals though.

That sounds good, thanks for the explanation!
Duplicate of this bug: 1424875
Component: Migration → Places
Product: Firefox → Toolkit
Attachment #8935484 - Flags: review?(mak77) → review?(standard8)
Comment on attachment 8935484 [details]
Bug 1423612 - Pass VisitData by reference

https://reviewboard.mozilla.org/r/205984/#review213346

Looks good. Thanks.
Attachment #8935484 - Flags: review?(standard8) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ae94696f8a1a -d 41a40a4edb54: rebasing 439577:ae94696f8a1a "Bug 1423612 - Pass VisitData by reference r=standard8" (tip)
merging toolkit/components/places/History.cpp
warning: conflicts while merging toolkit/components/places/History.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf435909ca6a
Pass VisitData by reference r=standard8
https://hg.mozilla.org/mozilla-central/rev/bf435909ca6a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.