Closed Bug 1211736 Opened 9 years ago Closed 7 years ago

Investigate explicitly opting in to skipping bounds checks to avoid perf regressions from bug 1159244

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox44 --- affected

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(3 files)

Bug 1159244 hasn't been able to land at least in part due to perf regressions. Maybe by allowing callers to explicitly opt in to skipping the bounds checks we could get the benefits of safety in the majority of our call sites. I've written a crude logging tool to analyze where we call ElementAt from. (It looks like RemoveElementsAt is also being changed so that should figure in there.)
The moz.build changes are because at some point stand alone WebRTC tests were having link errors with my ElementAt logging. I'm not sure they are still needed.
Attached file crude analyzer script
This analyzes the output of the patch and prints out the most frequent call sites. It produces output that looks like this:

Total number of stacks seen: 708305

171516 UNSAFE XPCJSContextStack::Peek() (/home/amccreight/mc/js/xpconnect/src/xpcprivate.h:2866)
101926 UNSAFE mozilla::HaveExistingCacheFor(void*) (/home/amccreight/mc/modules/libpref/Preferences.cpp:198)
57704 SAFE mozilla::DeadlockDetector<mozilla::BlockingResourceBase>::InTransitiveClosure(mozilla::DeadlockDetector<mozilla::BlockingResourceBase>::OrderingEntry const*, mozilla::DeadlockDetector<mozilla::BlockingResourceBase>::OrderingEntry const*) const (/tmp/../../dist/include/mozilla/DeadlockDetector.h:291)
38918 UNSAFE CallMethodHelper::GetDispatchParam(unsigned char) (/home/amccreight/mc/js/xpconnect/src/XPCWrappedNative.cpp:1320)
20660 UNSAFE nsXMLContentSink::GetCurrentStackNode() (/home/amccreight/mc/dom/xml/nsXMLContentSink.cpp:837)

(The logging should ideally use sampling...)
I took a quick look at the top two.

> 171516 UNSAFE XPCJSContextStack::Peek()
> (/home/amccreight/mc/js/xpconnect/src/xpcprivate.h:2866)
This call-site is obviously correct, and if it's causing a big performance hit it should be an easy win to simply use the unsafe version.  That being said, I have a hard time believing that this check isn't optimized away in a real optimized build.

> 101926 UNSAFE mozilla::HaveExistingCacheFor(void*)
> (/home/amccreight/mc/modules/libpref/Preferences.cpp:198)
This isn't even present in an optimized build, so we can ignore it.
Attached file TART frequent stacks
I modified my ElementAt() profiler to only take a stack every 107 calls, then I ran the TART test for a few minutes until I got bored. TART is one of the tests that regressed. Here's the top calls sites for that, which might be useful. There's a lot of layout stuff in there. Ideally, I'd have just run some of the subtests that regressed the most but I'm not sure how to do that. Not surprisingly, there's some layout stuff in there.

nsDisplayListBuilder::CurrentPresShellState() looks at the last element of an array. This is similar to XPCJSContextStack::Peek(), but note that in nsDisplayListBuilder the code does not locally test for len == 0, so the compiler may not be able to optimize away this check.

FrameLayerBuilder::GetDisplayItemData() is iterating over an array, so at first glance it looks safe. Note that Length() returns size_type, which I think will be uint64_t, and |i| here has type uint32_t. I guess at worst that would just cause an infinite loop, so that shouldn't prevent optimizing out the bounds check. The same thing for GetDisplayItemDataForManager.
(In reply to Andrew McCreight [:mccr8] from comment #4)
> FrameLayerBuilder::GetDisplayItemData() is iterating over an array, so at
> first glance it looks safe. Note that Length() returns size_type, which I
> think will be uint64_t, and |i| here has type uint32_t.

This is not the greatest interface, but Length will always fit into a uint32_t, even on 64-bit machines; see the definition of nsTArray's Header data structure:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#235

as the capacity of the array always has to be 31-bit unsigned.
This landed without the need for any shenanigans. Hooray!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: