TISGetInputSourceProperty is 12.3% of my startup path

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: joelr, Assigned: masayuki)

Tracking

Trunk
mozilla1.9.3a1
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 .4-fixed)

Details

(Whiteboard: [ts])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Can the input bits in the nsChildView constructor be backgrounded in another thread or kicked down the road? 

	0.0%	12.4%	XUL	                                 nsView::LoadWidget(nsID const&)
	0.0%	12.4%	XUL	                                  CallCreateInstance(nsID const&, nsISupports*, nsID const&, void**)
	0.0%	12.4%	XUL	                                   nsComponentManagerImpl::CreateInstance(nsID const&, nsISupports*, nsID const&, void**)
	0.0%	12.4%	XUL	                                    nsChildViewConstructor(nsISupports*, nsID const&, void**)
	0.0%	12.4%	XUL	                                     nsChildView::nsChildView()
	0.0%	12.3%	HIToolbox	                                      TISGetInputSourceProperty
	0.0%	12.3%	HIToolbox	                                       TSMGetInputSourceProperty
	0.0%	12.3%	HIToolbox	                                        islGetInputSourceProperty
	0.0%	11.9%	HIToolbox	                                         islSetKLPropertyForInputSource
	0.0%	11.5%	HIToolbox	                                          islPopulateKLPropertiesFromBundle
(Reporter)

Updated

9 years ago
Whiteboard: [ts]
(Reporter)

Comment 1

9 years ago
bz:nsCommandLine::Run 15%, this is more interesting to me
bz:calling openWindow on the widnow watcher
bz:TISGetInputSourceProperty 12.3%, what the heck is that and can we avoid it?
bz:it's called from nsChildView ctor and seems to be OS code
bz:parseplist, etc
joshmoz:bz: it is OS code, masayuki added it recently
joshmoz:bz: we have to start using TIS for intl input on mac
bz:well, it's being 12% of the main-thread non-idle startup time in this profile
joshmoz:bz: but it can only be used on 10.5 +, which is why you'll only see that on trunk
bz:do we have to do it quite this early in startup? and do we have to do it on the main thread?
joshmoz:bz: that's a question for masayuki, can you file a bug asking those two things?
bz:joelr: ^
joelr:bz: will file a bug
The TIS related code in nsChildView::nsChildView is only needed at debugging, so, we don't need to run them at the starting up for the daily use.

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsChildView.mm#94
> 94 #ifdef MOZ_LOGGING
> 95 #define FORCE_PR_LOG
> 96 #endif

Looks like that the logging is enabled on the release build too.

I think that #671-#713 should be built only at debug build, and it should be enough.
blocking2.0: --- → ?
Created attachment 401785 [details] [diff] [review]
Patch v1.0

simple fix.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #401785 - Flags: review?(joshmoz)
Created attachment 401786 [details] [diff] [review]
Patch v1.0.1

Ugh... the indent was changed by xcode at pasting the code :-(
Attachment #401785 - Attachment is obsolete: true
Attachment #401786 - Flags: review?(joshmoz)
Attachment #401785 - Flags: review?(joshmoz)

Updated

9 years ago
Attachment #401786 - Flags: review?(joshmoz) → review+
http://hg.mozilla.org/mozilla-central/rev/1957eb182db3
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Current trunk is using the Leopard stuff, no?
(In reply to comment #8)
> Current trunk is using the Leopard stuff, no?

Really? If so, we can remove the code for 10.4.
No, we can't.  We're only building on 10.5 at the moment, but not purposefully breaking 10.4 yet.
(Reporter)

Comment 11

9 years ago
Did Ts really go down 30% on this one fix?
(In reply to comment #10)
> No, we can't.  We're only building on 10.5 at the moment, but not purposefully
> breaking 10.4 yet.

Hrm, ok.

(In reply to comment #11)
> Did Ts really go down 30% on this one fix?

I'm not sure. The removed code calls TIS APIs very many times because the count of the installed keyboard layouts.

And I think that we should try to land the patch to 1.9.2 branch too. The non-Leopard path outputted similar log with other APIs (KL*). And also the change is just removing the code from the release builds, so, the risk is very low. If we can improve the ts performance, it's great thing.
Created attachment 402038 [details] [diff] [review]
Patch for 1.9.2 branch v1.0
Attachment #402038 - Flags: review?(joshmoz)
oops, posted the request without comments, sorry.

The patch also removes the debugging code from 1.9.2 branch's release build. The risk is low but I'm not sure whether the patch improves how much ts value. However, I think we should try to improve the ts value on 1.9.2 branch too.

Updated

9 years ago
Attachment #402038 - Flags: review?(joshmoz) → review+
Attachment #402038 - Flags: approval1.9.2?

Updated

9 years ago
Attachment #402038 - Flags: approval1.9.2? → approval1.9.2+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0f70085df9a4
blocking2.0: ? → ---
status1.9.2: --- → beta1-fixed

Comment 17

9 years ago
I think this can go on 1.9.1, if so we should do it.
(In reply to comment #17)
> I think this can go on 1.9.1, if so we should do it.

Yes, but I want to wait the ts tests. However, unfortunately, the tests were not started by my check-in :-(

Can somebody start the perf tests on 3.6's tinderbox forcedly?
Duplicate of this bug: 517915
Comment on attachment 402038 [details] [diff] [review]
Patch for 1.9.2 branch v1.0

Approved for 1.9.1.4, a=dveditz
Attachment #402038 - Flags: approval1.9.1.4? → approval1.9.1.4+
status1.9.1: --- → .4-fixed
You need to log in before you can comment on or make changes to this bug.