Open Bug 1332680 Opened 8 years ago Updated 2 years ago

Devirtualize More Methods/Classes with Final Keywords

Categories

(Core :: General, defect, P3)

Product:
Core
Component:
General
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox53 --- affected

People

(Reporter: tjr, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [overhead:noted])

Attachments

(2 files, 1 obsolete file)

suggest-final-methods.log
868.46 KB, text/plain
Details
suggest-final-types.log
325.80 KB, text/plain
Details
Repeat the process in #1047696 to add final keywords. Eliminate vtable lookups and a layer of indirection and also reduce the number of targets for UAF exploitation. (Performance and security gains in one!)
Depends on: 1330608
Blocks: 1332682
Depends on: 620058
Depends on: 1360299
Depends on: 1360300
Depends on: 1360301
No longer depends on: 1360301
No longer depends on: 1360300
No longer depends on: 1360299
We do gcc builds for Linux. So we don't need MinGW to do this, we just need to get the warnings from our GCC builds and do something about them!
No longer depends on: 1330608
After discussions in SF, the path forward for this is to add -lto and rerun the analysis, and see if we can get the build to complete and give us data.
Attachment #8869560 - Attachment is obsolete: true
Okay, I got a successfull LTO build, and extracted out these warnings. I've attached the raw output, and I'm going to paste some high-hit ones here that aren't in third party code: > 483 /dom/base/nsGlobalWindowInner.h:206:7: warning: Declaring type 'struct nsGlobalWindowInner' final would enable devirtualization of 483 calls > 143 /dom/base/nsGlobalWindowOuter.h:164:7: warning: Declaring type 'struct nsGlobalWindowOuter' final would enable devirtualization of 143 calls > 70 /netwerk/cache2/CacheFileIOManager.h:46:7: warning: Declaring type 'struct CacheFileHandle' final would enable devirtualization of 70 calls > 69 /netwerk/cache2/CacheFileIOManager.h:262:7: warning: Declaring type 'struct CacheFileIOManager' final would enable devirtualization of 69 calls > 603 /dom/promise/Promise.cpp:66:0: warning: Declaring method 'Release' final would enable devirtualization of 603 calls > 433 /media/webrtc/signaling/src/jsep/JsepSessionImpl.h:151:0: warning: Declaring method 'GetTransceivers' final would enable devirtualization of 433 calls > 379 /xpcom/threads/AbstractThread.cpp:197:1: warning: Declaring method 'Release' final would enable devirtualization of 379 calls > 249 /xpcom/threads/AbstractThread.cpp:197:1: warning: Declaring method 'Release' final would enable devirtualization of 249 calls > 207 /xpcom/threads/AbstractThread.cpp:197:1: warning: Declaring method 'AddRef' final would enable devirtualization of 207 calls > 189 /layout/base/nsPresContext.cpp:439:0: warning: Declaring method 'Release' final would enable devirtualization of 189 calls > 154 /dom/file/Blob.cpp:45:0: warning: Declaring method 'Release' final would enable devirtualization of 154 calls > 141 /obj-firefox/ipc/ipdl/PContentChild.cpp:4712:0: warning: Declaring method 'GetIPCChannel' final would enable devirtualization of 141 calls > 124 /xpcom/threads/AbstractThread.cpp:197:1: warning: Declaring method 'AddRef' final would enable devirtualization of 124 calls > 121 /dom/base/nsGlobalWindowInner.cpp:4487:0: warning: Declaring method 'GetExistingListenerManager' final would enable devirtualization of 121 calls > 118 /dom/base/nsGlobalWindowInner.cpp:4476:0: warning: Declaring method 'GetOrCreateListenerManager' final would enable devirtualization of 118 calls > 116 /dom/promise/Promise.cpp:65:0: warning: Declaring method 'AddRef' final would enable devirtualization of 116 calls > 102 /dom/media/MediaStreamTrack.cpp:199:0: warning: Declaring method 'Release' final would enable devirtualization of 102 calls > 100 /dom/media/MediaStreamTrack.cpp:199:0: warning: Declaring method 'Release' final would enable devirtualization of 100 calls > 95 /layout/base/nsPresContext.cpp:438:0: warning: Declaring method 'AddRef' final would enable devirtualization of 95 calls > 83 /image/imgRequestProxy.cpp:99:0: warning: Declaring method 'Release' final would enable devirtualization of 83 calls
smaug might be interested in looking at the list. There's a lot of DOM stuff in comment 9, and he hates virtual calls. :)
I have a try run going for nsGlobalWindowInner/Outer here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48615cf2083ae24a28f7981cb2ec2de42abc3ae8
It looks like we may need new variants of the ISUPPORTS decl macros that have final in them. (I hadn't realized you could declare individual methods final, but it makes sense.)
I am glad this warning seems useful. If you do LTO+PGO build you will get the warnings sorted by the actual number of executions of the devirtualized calls :)
Blocks: 1443252
Attachment #8955498 - Attachment mime type: text/x-log → text/plain
Attachment #8955501 - Attachment mime type: text/x-log → text/plain
Depends on: 1444175
Depends on: 1444490
Depends on: 1446509
Blocks: 1449288
Whiteboard: [overhead:noted]
Priority: -- → P3
See Also: → 1517816
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: