517549 - TISGetInputSourceProperty is 12.3% of my startup path

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Joel Reymont (:joelr)]

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

Comment 1

[Joel Reymont (:joelr)]

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

Comment 2

[Joel Reymont (:joelr)]

This is the place to look:

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsChildView.mm#658

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

simple fix.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0.1

Ugh... the indent was changed by xcode at pasting the code :-(

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/1957eb182db3

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like the patch improves the ts value of the current trunk, so, the KL* APIs are expensive too. I should create a patch for 1.9.2 branch too, probably.

http://graphs.mozilla.org/graph.html#tests=[{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22169%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22173%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22178%22}]&sel=1252833380,1253542320

Comment 8

[Boris Zbarsky [:bzbarsky]]

Current trunk is using the Leopard stuff, no?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #8)
> Current trunk is using the Leopard stuff, no?

Really? If so, we can remove the code for 10.4.

Comment 10

[Boris Zbarsky [:bzbarsky]]

No, we can't.  We're only building on 10.5 at the moment, but not purposefully breaking 10.4 yet.

Comment 11

[Joel Reymont (:joelr)]

Did Ts really go down 30% on this one fix?

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://groups.google.com/group/mozilla.dev.platform/msg/284b13f7ede1cdb9 says we already broke 10.4.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for 1.9.2 branch v1.0

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0f70085df9a4

Comment 17

[Josh Aas]

I think this can go on 1.9.1, if so we should do it.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 402038 [details] [diff] [review]
Patch for 1.9.2 branch v1.0

http://graphs.mozilla.org/graph.html#tests=[{%22test%22:%2216%22,%22branch%22:%2210%22,%22machine%22:%22163%22},{%22test%22:%2216%22,%22branch%22:%2210%22,%22machine%22:%22156%22},{%22test%22:%2216%22,%22branch%22:%2210%22,%22machine%22:%22178%22},{%22test%22:%2216%22,%22branch%22:%2210%22,%22machine%22:%22175%22}]&sel=1253512907,1253606640

The test results were coming. Looks like that the patch cannot improve the ts performance on 10.4, but it improves it on 10.5. So, we should land the patch to 1.9.1 too.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 402038 [details] [diff] [review]
Patch for 1.9.2 branch v1.0

Approved for 1.9.1.4, a=dveditz

Comment 22

[Mark Banner (:standard8)]

FTR this landed on 1.9.1.4: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/159c10b73442