Closed Bug 1417266 Opened 7 years ago Closed 7 years ago

GenerateGUID responsible for ~25% of history import times in large profiles

Categories

(Firefox :: Migration, enhancement, P3)

Product:
Firefox
Component:
Migration
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: alexical, Assigned: alexical)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Reuse HCRYPTPROV
6.28 KB, patch
Details | Diff | Splinter Review
Bug 1417266 - Cache calls to CryptAcquireContext
59 bytes, text/x-review-board-request
mak
: review+
Details 
About 25% of the time (measured with patches from Bug 1414892 applied) taken to import a large profile's history is spend generating GUIDs. Almost all of this time is spent in CryptAcquireContext and CryptGenRandom on Windows. Do we need these guids to use a cryptographically secure RNG? Can we just use a cheaper RNG, or even just seed the value with the output of a crypto RNG and hash it for each record we insert?
Flags: needinfo?(mak77)
(In reply to Doug Thayer [:dthayer] from comment #0)
> About 25% of the time (measured with patches from Bug 1414892 applied) taken
> to import a large profile's history is spend generating GUIDs. Almost all of
> this time is spent in CryptAcquireContext and CryptGenRandom on Windows. Do
> we need these guids to use a cryptographically secure RNG?

No, not strictly, what we need is uniqueness of results (low collision rate), we don't care about predictability.

> Can we just use a
> cheaper RNG, or even just seed the value with the output of a crypto RNG and
> hash it for each record we insert?

We should measure the collision rate to tell.

Possible alternatives I'm thinking about:
1. We could store the cryptoProvider and just release it on shutdown, rather than acquire it every single time, this would remove half of the work
2. we could experiment with the other providers (apart from PROV_RSA_FULL) and see if they provide comparable (for uniqueness) but faster results.
3. we could experiment with CoCreateGuid, provided we will have to crop its result to 72 bits (IIRC we use 9 bytes currently) and that may have an effect on uniqueness, depending on the way we transform/crop.
Flags: needinfo?(mak77)
Attached patch Reuse HCRYPTPROV (obsolete) — — Splinter Review 
Almost all of the time here is in getting the HCRYPTPROV. I was looking at this earlier and I have a preliminary patch to work around this which I'm attaching, it saved 12% in my tests (before Doug's bug 1414892 patch, so that matches up nicely with 25% after).
I'd rather not have so much machinery around releasing the provider as I had to add, and there's a really sloppy use of malloc/free in there that should probably be something more appropriate.
Attached patch Reuse HCRYPTPROV — — Splinter Review 
Clean up some junk from other work.
Attachment #8928597 - Attachment is obsolete: true
How expensive is the call? I was actually thinking we could provide a cryptoProvider getter from nsNavHistory (storing it as a member of the service), and destroy it when the history service goes away. That would simplify some of this boilerplate and would make guids generation faster not just for History.cpp, but also for bookmarks (we have bulk bookmarks additions too).

Though, that may have some thread-safety implications, for which we could
1. create it on startup on the main-thread and pass it out through a const method. That has a possible startup cost (thus the question).
2. create one context per thread (main thread and helper thread), though we couldn't just pass it out from const nsNavHistory, since the lazy getter would need to store the context.
Priority: -- → P3
Here's an profile from quantum reference hw: https://perfht.ml/2hEQSDI (this is from the current Nightly and thus includes bug 141892)

Out of 1168ms spent in InsertVisitedURIs::Run for 2000 history entries, GenerateGUID is 264ms, assuming all of that is CryptAcquireContext (which I think is fairly close) each call runs around .1ms. (I doubled the times, as profiling on Windows with 1ms sampling regularly gives numbers about half the actual duration). It's 22% of the time here.
Then looks like something we could just do on history service start. The only remaining doubt I have is whether that may cause a system library to be initialized, but I assume this library will be used shortly after startup by anything using crypt and many other software (also by Win itself), so in practice I'm probably just overzealous.
I'd suggest to try just doing that, init the context in the nsNavHistory constructor, and destroy it in the destructor, so we are sure both will happen on the same thread.
Expose a const method to get the context, it will be used from a const history pointer got through GetConstHistoryService(), that we can use on both threads (GenerateGUID can be used on both main and helper threads).
That should potentially make a much simpler patch, provided I didn't miss something.
Blocks: 1416788
(In reply to Doug Thayer [:dthayer] from comment #7)
> Created attachment 8930601 [details]
> Bug 1417266 - Cache calls to CryptAcquireContext
> 
> The details of the caching the result of CryptAcquireContext is
> relatively straightforward, however moving the <windows.h> include
> involved trying to work around its GetMessage definition when
> working with mozIStorageError.

Uh, GetMessage in Storage is a method, how can it conflict with a winapi? :(

WinUtils.h seems to already take care of the GetMessage problem, thus it could be enough to include that instead of windows.h?
Otherwise, would it help to move the whole GenerateRandomBytes elsewhere? like directly in nsNavHistory.cpp (instead of exposing the context, directly expose a const method to generate random bytes and internally use the stored context).
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Comment on attachment 8930601 [details]
Bug 1417266 - Cache calls to CryptAcquireContext

https://reviewboard.mozilla.org/r/201716/#review207726

::: toolkit/components/places/Helpers.cpp:57
(Diff revision 2)
>  #ifdef DEBUG
>    int32_t result;
>    nsresult rv = aError->GetResult(&result);
>    NS_ENSURE_SUCCESS(rv, rv);
>    nsAutoCString message;
> +

leftover newline?

::: toolkit/components/places/nsNavHistory.h:498
(Diff revision 2)
>      return mBatchLevel > 0;
>    }
>  
> +#if defined(XP_WIN)
> +
> +  /**

nit: please remove the empty line after the #if defined(XP_WIN)

::: toolkit/components/places/nsNavHistory.h:507
(Diff revision 2)
> +    if (mCryptoProviderInitialized) {
> +      aCryptoProvider = mCryptoProvider;
> +      return NS_OK;
> +    } else {
> +      return NS_ERROR_FAILURE;
> +    }

NS_ENSURE_STATE(mCryptoProviderInitialized);
aCryptoProvider = mCryptoProvider;
return NS_OK;

::: toolkit/components/places/nsNavHistory.h:670
(Diff revision 2)
> +
> +  // Used to cache the call to CryptAcquireContext, which is expensive
> +  // when called thousands of times
> +#if defined(XP_WIN)
> +  HCRYPTPROV mCryptoProvider;
> +  bool mCryptoProviderInitialized;

please initialize to false in the nsNavHistory constructor

::: toolkit/components/places/nsNavHistory.cpp:316
(Diff revision 2)
>    if (gHistoryService == this)
>      gHistoryService = nullptr;
> +
> +#if defined(XP_WIN)
> +  if (mCryptoProviderInitialized) {
> +    (void)CryptReleaseContext(mCryptoProvider, 0);

"Unused << " instead of "(void)"
Attachment #8930601 - Flags: review?(mak77) → review+
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25f4bfe00549
Cache calls to CryptAcquireContext r=mak
https://hg.mozilla.org/mozilla-central/rev/25f4bfe00549
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: