1596398 - Search engine/settings cache is rewritten on every startup, regardless of if there are changes or not.

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Mark Banner (:standard8)]

Currently, when the search service starts up, or reloads engines (after a configuration change), the search cache is rebuilt and written to disk, regardless of if any values have actually changed since the previous version.

On my machine, the compressed size is generally around 2.2kB. The maximum it goes up to is around 14kB.

Although it is small, it is still an unwanted write that we're doing on startup, which has a small chance of leading to extra errors.

We should be able to at least iterate through the objects and compare them to check if they're the same or not. The more expensive solution would be to separate out the cache handling into its own object, and track when items are actually changed or not.

Comment 1

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #0)

Currently, when the search service starts up, or reloads engines (after a configuration change), the search cache is rebuilt and written to disk, regardless of if any values have actually changed since the previous version.

Isn't there always at least one value that has changed when rebuilding the cache? Eg. the build id.
I thought we had xpcshell tests ensuring we don't rebuild the cache when nothing has changed.

Comment 2

[Mark Banner (:standard8)]

(In reply to Florian Quèze [:florian] from comment #1)

(In reply to Mark Banner (:standard8) from comment #0)

Currently, when the search service starts up, or reloads engines (after a configuration change), the search cache is rebuilt and written to disk, regardless of if any values have actually changed since the previous version.

Isn't there always at least one value that has changed when rebuilding the cache? Eg. the build id.

The build ID can remain consistent for at least a few weeks if there's no new release. The only other thing could be the expiry of the search engine default lookup from ABSearch - which I think currently gets set to a day - but if you restart the browser twice in one day, that's still something extra, and with modernisation it'll soon be going away.

I thought we had xpcshell tests ensuring we don't rebuild the cache when nothing has changed.

I've not seen any tests for this.

Comment 3

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #2)

I thought we had xpcshell tests ensuring we don't rebuild the cache when nothing has changed.

I've not seen any tests for this.

I had https://searchfox.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_geodefaults.js in mind, but it seems to only check that we don't do needless network requests, not that we don't do pointless cache rebuilds. We should have a test ensuring https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/toolkit/components/search/SearchService.jsm#1169 doesn't break in a way that would make the test always true.

Comment 4

[Mark Banner (:standard8)]

I've looked into this a bit. With our current architecture, the easiest way to check for detection is to do a difference of the cache objects before writing. Although it'll take a little of work, it will prevent the potentially more expensive write.

However, at the moment, our cache sub-objects, e.g. engines, metadata, are shared 'pointers' between the cache to the engine objects themselves. So the diff fails to find new changes.

There's probably a few things to do.

1. When reading the cache, only return the engine details, not the full cache. After the legacy configuration is removed, the search code doesn't need the full cache information so we shouldn't return all of it.

• This will also need the corruption checks moving into the SearchCache, but I that's a better place to handle them anyway.

2. Make sure we clone objects when returning them from the cache (or when inserting into engines), so that we can do clean comparisons.
3. Do the cache comparisons up front, before queuing writes, e.g. if the engine says it is modified, but the data we store hasn't changed, there's no point in queuing a task for it.

Making this depend on bug 1619926 - the removal of the legacy code, it may also depend on bug 1637744 as it might be easier to have those tidied up as well.

Comment 5

[Mark Banner (:standard8)]

I'm starting to think that rather than having a get function which returns the object for the user's saved settings, we should possibly have one which returns the (cloned) engine information.

We should also consider if it is better not to read the settings every time we do the equivalent of get - we shouldn't need to reload the settings file when reloading engines, as it should already be in memory.

Comment 6

[Mandy Cheang [:mcheang][she/her]]

In regards to Comment 5,

We should also consider if it is better not to read the settings every time we do the equivalent of get - we shouldn't need to reload the settings file when reloading engines, as it should already be in memory.

We had a quick discussion/spike on this and we concluded that we're reading the settings from disk when reloading engines because there could be a possible update of the engines. On reload, iirc, there are no notifications sent to the observers to let us know if there was a change to the engines.

I am closing this because Bug 1779094 fixes the issue of "Search Settings is rewritten on every startup, regardless of if there are changes or not."
Bug 1779094 checks to see if there's a change, if there is, we write to disk. Otherwise we don't write to disk.