1456888 - Use Linux filesystem type to decide about NSS SQL DB caching and speed measuring

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Kai Engert (:KaiE:)]

As described in bug 1432484 users may experience slow NSS performance when storing the NSS SQL DB on a slow network filesystem.

The workaround is to manually define environment variable NSS_SDB_USE_CACHE, which may be used to explicitly to automatically enable caching.

Firefox has implemented bug 1444943 which enables caching automatically if the NSS DB is stored on an NFS filesystem.

I like the approach that has been discovered in that Firefox bug, but I think the detection should be extended to cover a wider set of filesystems (not just NFS), and in addition I think the filesystem type check should be done at the NSS code level, to help as many applications as possible,

Comment 1

[Kai Engert (:KaiE:)]

Attachment: 1456888-v1.patch

Initial patch for discussion. Only Linux so far.

Comment 2

[Kai Engert (:KaiE:)]

Attachment: 1456888-v2.patch

Comment 3

[Kai Engert (:KaiE:)]

The current patch focuses on Linux. I understand the Windows code is still being worked on in 1444511. We could further update the NSS code at a later time, after the Windows logic in Firefox was completed.

Comment 4

[Kai Engert (:KaiE:)]

try build seems fine: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=664303b01a577519a20121d0aa774ac7aa5dd9a6

Comment 5

[Robert Relyea]

Comment on attachment 8971162 [details] [diff] [review]
1456888-v2.patch

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

The code is correct as is. I've made a few comment you can choose to address or not.

bob

::: lib/softoken/sdb.c
@@ +1953,5 @@
> +                case NFS_SUPER_MAGIC:
> +                    /* We assume these are slow. */
> +                    enableCache = PR_TRUE;
> +                    break;
> +                case AFS_SUPER_MAGIC:

I would expect AFS is actually enableCache=PR_FALSE. AFS does full file caching, which means once you've read the file it's there in your local filesystem, so there isn't a per access cost. OTOH, AFS is rare enough, measuring the output is fine. (actually if you are really sharing the file on AFS, then there is reason to believe sqlite will run into cache consistency issues because AFS doesn't write changes back until close, so it's universally recognized as inappropriate for database access.

@@ +1970,5 @@
> +#endif
> +    }
> +
> +    /* If we have already decided to enable caching, then we don't measure. */
> +    if (!enableCache && measureSpeed) {

Don't need to check for !enableCache. We only set measureSpeed if we haven't dealt with the cache. (that is when we know we want to do the automatic access we always set measureSpeed explicitly, so the !enableCache is a noop).

Comment 6

[Kai Engert (:KaiE:)]

Thanks for the review. I'll add Bob's requested changes.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: 1456888-v3.patch

Comment 8

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/5a210945d248