1842079 - Three seconds of CPU work during startup in mozilla::safebrowsing::ProtocolParserProtobuf::ProcessEncodedAddition and mozilla::safebrowsing::Classifier::UpdateTableV4

Status: NEW

(Toolkit :: Safe Browsing, enhancement, P3)

Description

[Markus Stange [:mstange]]

Profile: https://share.firefox.dev/439ZAKV

I ran mozregression --launch 2023-06-20 and took a startup profile on Windows, and noticed 3 seconds of CPU work in safebrowsing code.

This seems like a lot of time. Can we reduce it?

Comment 1

[Markus Stange [:mstange]]

Replacing NS_QuickSort with a sort that inlines the comparator call will probably help significantly for the ProcessEncodedAddition part: https://share.firefox.dev/3XYm9By

Comment 2

[Jens Stutte [:jstutte]]

That looks like a very long sort indeed. I'd be curious if apart from the non-inlining penalty we also see some bad quadratic edge case behavior of quick sort here. Do you have a reproduction case for this? You could try to apply the patch stack from bug 1839051 and bug 1842130 and rerun your case with it.

Comment 3

[Jens Stutte [:jstutte]]

(In reply to Jens Stutte [:jstutte] from comment #2)

That looks like a very long sort indeed.

It's not just one sort invocation, there are quite a few in a row. All this code looks weird, especially the endian handling and comparisons.

Comment 4

[Jens Stutte [:jstutte]]

Looking at the profile, it seems we spend about 50% of the sorting time with calls to LdrpDispatchUserCallTarget. If this is from some Windows Control Flow Guard (first keyword google found for me) around recursive function calls, the gain of switching to std::sort could be quite huge here, as I assume the compiler will just inline all the template code, avoiding any recursion or other function calls. Is this something that will always happen on Windows or was this a special build?

Comment 5

[Markus Stange [:mstange]]

(In reply to Jens Stutte [:jstutte] from comment #4)

If this is from some Windows Control Flow Guard (first keyword google found for me) around recursive function calls

It affects all indirect function calls, e.g. calls via a function pointer or calls to virtual methods.

the gain of switching to std::sort could be quite huge here, as I assume the compiler will just inline all the template code, avoiding any recursion or other function calls.

I agree!

Is this something that will always happen on Windows or was this a special build?

This was a regular Firefox Nightly build. I'm not sure if it's something we explicitly enabled or if it's always used.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dimi, could you have a look please?

For more information, please visit BugBot documentation.

Comment 7

[Alex Thayer [:alexical] (she/her)]

The Performance Impact Calculator has determined this bug's performance impact to be low. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: [x] Windows [x] macOS [x] Linux
Websites affected: Rare
[x] Causes severe resource usage

Comment 8

[Markus Stange [:mstange]]

New profile with 2 seconds of 100% CPU: https://share.firefox.dev/3OF16Ai

This profile also shows that other network requests don't complete while nsUrlClassifierDBServiceWorker::FinishStream is running.

Comment 9

[Jens Stutte [:jstutte]]

(In reply to Markus Stange [:mstange] from comment #1)

Replacing NS_QuickSort with a sort that inlines the comparator call

This happened a while ago, and before the next profile was recorded:

(In reply to Markus Stange [:mstange] from comment #8)

New profile with 2 seconds of 100% CPU: https://share.firefox.dev/3OF16Ai

This profile also shows that other network requests don't complete while nsUrlClassifierDBServiceWorker::FinishStream is running.

I see quite some nsTArray_Impl::ElementAt in that profile, maybe bug 1887697 might help a bit here.