1376471 - nsNavHistory does unnecessary hashtable lookups

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P3)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Just use Put rather than Get+Remove+Put when an entry already exist

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8881419 [details] [diff] [review]
Just use Put rather than Get+Remove+Put when an entry already exist

This patch slightly changes the condition for when ExpireNonrecentEvents
is triggered.

Before:
if (Count() == *_MAX_LENGTH + 1) when the method is called, Get+Remove
would remove the entry if it exists and thus make the condition false

if (Count() == *_MAX_LENGTH) when the method is called,
the condition is always false, even if we add a new entry

After:
if (Count() == *_MAX_LENGTH + 1) when the method is called,
the condition will be true whether or not the entry exist

if (Count() == *_MAX_LENGTH) when the method is called,
the condition will become true if we're adding a new entry 

IOW, the old behavior is that "*_MAX_LENGTH + 1" is allowed without
triggering ExpireNonrecentEvents.  The new behavior is that
"*_MAX_LENGTH + 1" entries always triggers ExpireNonrecentEvents.
I suspect the old behavior is actually a bug.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8881419 [details] [diff] [review]
Just use Put rather than Get+Remove+Put when an entry already exist

Review of attachment 8881419 [details] [diff] [review]:
-----------------------------------------------------------------

Passing this review off because I don't have enough familiarity with the Places code to say whether this is valid or not, see below.

::: toolkit/components/places/nsNavHistory.cpp
@@ -1109,5 @@
>  
>    if (mRecentBookmark.Count() > RECENT_EVENT_QUEUE_MAX_LENGTH)
>      ExpireNonrecentEvents(&mRecentBookmark);
>  
> -  mRecentBookmark.Put(uriString, GetNow());

I don't know if moving this call (and the other, similar cases below) before the ExpireNonrecentEvents call is valid.  Maybe ExpireNonrecentEvents relies on uriString not existing in the hashtable somehow...?

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8881419 [details] [diff] [review]
Just use Put rather than Get+Remove+Put when an entry already exist

Review of attachment 8881419 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the question answered positively. From a quick look at http://searchfox.org/mozilla-central/source/xpcom/ds/nsBaseHashtable.h#151 it sounds like ok to me.

::: toolkit/components/places/nsNavHistory.cpp
@@ +1101,5 @@
>    nsAutoCString uriString;
>    nsresult rv = aURI->GetSpec(uriString);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  mRecentBookmark.Put(uriString, GetNow());

Does Put() actually replace the value if the key is present, or does it skip setting the value if the key exists?
I'm asking exactly because of ExpireNonrecentEvents, each entry has an expiration time, and we should be sure the expiration time for this uri gets replaced, or it may be expired immediately.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8881419 [details] [diff] [review]
Just use Put rather than Get+Remove+Put when an entry already exist

Thanks for the review.  Yes, if an hashtable entry already exists for the key,
then Put will replace its value.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c4016ee35f3
Just use Put rather than Get+Remove+Put when an entry already exist.  r=froydnj

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c4016ee35f3