Closed Bug 461566 Opened 14 years ago Closed 14 years ago

Don't call FindTearoff when not needed and cache XPCNativeInterfaces in quickstubs

Categories

(Core :: XPConnect, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: peterv, Assigned: peterv)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
A lot of callers of WrapNative know that the object they have implements the interface for which they want to wrap. We can avoid creating XPCWrappedNativeTearOff in those cases, but we must be careful not to do this for tearoffs to make sure those are kept alive by the wrapper.
Slightly changing the scope of this bug. Turns out it's hard to know when we can skip calling FindTearoff. The one case that seems certain is when wrapping for nsISupports. We could allow callers to pass in null for the IID, which would mean wrap for nsISupports, and then skip FindTearoff. The biggest cost in FindTearoff seems to be looking up the XPCNativeInterface (hash lookup with locking). Quickstubs have a known set of result types, so we can cache the XPCNativeInterfaces for all result types and avoid that cost most of the time. Patch coming up that does both these things.

FindTearoff is still showing up in profiles, but for now I don't think there's an easy way to avoid it.
Priority: -- → P2
Summary: Don't create XPCWrappedNativeTearOff when not needed → Don't call FindTearoff when not needed and cache XPCNativeInterfaces in quickstubs
Target Milestone: --- → mozilla1.9.1
Attached patch v2Splinter Review
Attachment #344659 - Attachment is obsolete: true
Attachment #349176 - Flags: superreview?(jst)
Attachment #349176 - Flags: review?(jst)
Attachment #349176 - Flags: superreview?(jst)
Attachment #349176 - Flags: superreview+
Attachment #349176 - Flags: review?(jst)
Attachment #349176 - Flags: review+
Comment on attachment 349176 [details] [diff] [review]
v2

Looks good! I like the fact that the interface table in dom_quickstubs.cpp only needs to contain interfaces for types that are ever returned, which means the list is surprisingly small.

r+sr=jst
We really want these performance improvements in 1.9.1.
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.1?
This is a significant performance gain in our JS -> C++ path, and I think we should take this before shipping. Blocking, and bumping to P1 as having this in a beta would be better than not.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: P2 → P1
http://hg.mozilla.org/mozilla-central/rev/037f635ced9f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b3
I had to back this out since it did not compile on windows:
nsScriptError.cpp
Building deps for /e/builds/buildbot/comm-central-win32/build/mozilla/js/src/xpconnect/src/nsScriptError.cpp
cl -FonsScriptError.obj -c  -DXPCOM_TRANSLATE_NSGM_ENTRY_POINT=1 -DMOZILLA_INTERNAL_API -D_IMPL_NS_GFX -D_IMPL_NS_MSG_BASE -D_IMPL_NS_WIDGET  -DMOZ_THUNDERBIRD=1 -DOSTYPE=\"WINNT5.2\" -DOSARCH=WINNT -DJSFILE -DJS_THREADSAFE -DEXPORT_XPC_API -DXPC_IDISPATCH_SUPPORT -I/e/builds/buildbot/comm-central-win32/build/mozilla/js/src/xpconnect/src/../loader  -I/e/builds/buildbot/comm-central-win32/build/mozilla/js/src/xpconnect/src -I. -I../../../../dist/include/xpcom -I../../../../dist/include/string -I../../../../dist/include/js -I../../../../dist/include/caps -I../../../../dist/include/necko -I../../../../dist/include/dom -I../../../../dist/include/content -I../../../../dist/include/editor -I../../../../dist/include/layout -I../../../../dist/include/rdf -I../../../../dist/include/svg -I../../../../dist/include/xuldoc -I../../../../dist/include/xultmpl -I../../../../dist/include   -I../../../../dist/include/xpconnect -Ie:/builds/buildbot/comm-central-win32/build/objdir-tb/mozilla/dist/include/nspr     -Ie:/builds/buildbot/comm-central-win32/build/objdir-tb/mozilla/dist/sdk/include       -GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fdgenerated.pdb  -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -O1 -MD            -D_CRT_SECURE_NO_DEPRECATE=1 -D_CRT_NONSTDC_NO_DEPRECATE=1 -DWINVER=0x500 -D_WIN32_WINNT=0x500 -D_WIN32_IE=0x0500 -DX_DISPLAY_MISSING=1 -DMOZILLA_VERSION=\"1.9.1b3pre\" -DMOZILLA_VERSION_U=1.9.1b3pre -DHAVE_SNPRINTF=1 -D_WINDOWS=1 -D_WIN32=1 -DWIN32=1 -DXP_WIN=1 -DXP_WIN32=1 -DHW_THREADS=1 -DSTDC_HEADERS=1 -DWIN32_LEAN_AND_MEAN=1 -DNO_X11=1 -DHAVE_MMINTRIN_H=1 -DHAVE_OLEACC_IDL=1 -DHAVE_ATLBASE_H=1 -DHAVE_WPCAPI_H=1 -D_X86_=1 -DD_INO=d_ino -DMOZ_EMBEDDING_LEVEL_DEFAULT=1 -DMOZ_EMBEDDING_LEVEL_BASIC=1 -DMOZ_EMBEDDING_LEVEL_MINIMAL=1 -DMOZ_BUILD_APP=../mail -DMOZ_XUL_APP=1 -DMOZ_DEFAULT_TOOLKIT=\"cairo-windows\" -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DACCESSIBILITY=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_NO_XPCOM_OBSOLETE=1 -DMOZ_OGG=1 -DMOZ_WAVE=1 -DMOZ_SYDNEYAUDIO=1 -DMOZ_MEDIA=1 -DMOZ_XTF=1 -DMOZ_CRASHREPORTER=1 -DMOZ_CRASHREPORTER_ENABLE_PERCENT=100 -DMOZ_ENABLE_CANVAS=1 -DMOZ_SVG=1 -DMOZ_UPDATE_CHANNEL=nightly -DMOZ_FEEDS=1 -DMOZ_STORAGE=1 -DMOZ_SAFE_BROWSING=1 -DMOZ_URL_CLASSIFIER=1 -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\"Mozilla\" -DMOZ_STATIC_BUILD=1 -DMOZ_TREE_CAIRO=1 -DHAVE_UINT64_T=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DMOZ_RDF=1 -DMOZ_MORK=1 -DMOZ_DLL_SUFFIX=\".dll\"  -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT /e/builds/buildbot/comm-central-win32/build/mozilla/js/src/xpconnect/src/nsScriptError.cpp
nsScriptError.cpp
e:\builds\buildbot\comm-central-win32\build\mozilla\js\src\xpconnect\src\xpcprivate.h(2768) : error C2332: 'struct' : missing tag name
e:\builds\buildbot\comm-central-win32\build\mozilla\js\src\xpconnect\src\xpcprivate.h(2768) : error C2011: '<unnamed-tag>' : 'enum' type redefinition
        D:\sdks\v6.0\include\shlwapi.h(1550) : see declaration of '<unnamed-tag>'
e:\builds\buildbot\comm-central-win32\build\mozilla\js\src\xpconnect\src\xpcprivate.h(2768) : error C2144: syntax error : '<unnamed-tag>' should be preceded by ')'
e:\builds\buildbot\comm-central-win32\build\mozilla\js\src\xpconnect\src\xpcprivate.h(2768) : error C2144: syntax error : '<unnamed-tag>' should be preceded by ';'
e:\builds\buildbot\comm-central-win32\build\mozilla\js\src\xpconnect\src\xpcprivate.h(2768) : error C2059: syntax error : ','
e:\builds\buildbot\comm-central-win32\build\mozilla\js\src\xpconnect\src\xpcprivate.h(2772) : error C2059: syntax error : ')'
e:\builds\buildbot\comm-central-win32\build\mozilla\js\src\xpconnect\src\xpcprivate.h(2772) : error C2238: unexpected token(s) preceding ';'
e:\builds\buildbot\comm-central-win32\build\mozilla\js\src\xpconnect\src\xpcprivate.h(2785) : error C2661: 'XPCConvert::NativeInterface2JSObject' : no overloaded function takes 10 arguments

http://hg.mozilla.org/mozilla-central/rev/1b45760df6ee
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Depends on: 495554
You need to log in before you can comment on or make changes to this bug.