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)
Core
XPCOM
Tracking
()
RESOLVED
WORKSFORME
Tracking | Status | |
---|---|---|
firefox44 | --- | affected |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(3 files)
806 bytes,
patch
|
Details | Diff | Splinter Review | |
1.85 KB,
text/x-python
|
Details | |
2.58 KB,
text/plain
|
Details |
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.)
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
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...)
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
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.
Comment 5•9 years ago
|
||
(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.
Assignee | ||
Comment 6•7 years ago
|
||
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.
Description
•