1337077 - PSpeechSynthesis::Msg_ReadVoicesAndState can take a long time

Status: RESOLVED FIXED

(Core :: Web Speech, defect)

Description

[(no longer active)]

<https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-02-05&keys=PSpeechSynthesis%253A%253AMsg_ReadVoicesAndState!PContent%253A%253AMsg_RpcMessage!PContent%253A%253AMsg_GetGfxVars!PAPZCTreeManager%253A%253AMsg_ReceiveMouseInputEvent&max_channel_version=nightly%252F54&measure=IPC_SYNC_LATENCY_MS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-02-02&table=0&trim=1&use_submission_date=0>

Start	End	IPC_SYNC_LATENCY_MS Count
0	1	0 (0%)
1	3	444 (9.4%)
3	8	935 (19.79%)
8	20	952 (20.15%)
20	50	830 (17.57%)
50	126	916 (19.39%)
126	317	449 (9.5%)
317	796	150 (3.17%)
796	2k	38 (0.8%)
2k	Infinity	11 (0.23%)

We should either send this initialization data asynchronously to the child or (if we need it during startup) send it synchronously on the command line.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Eitan, curious for your thoughts here.

Comment 2

[Eitan Isaacson [:eeejay]]

Ehsan, Two questions:

1. Am I reading the telemetry data. Not minimizing this problem, but do I understand correctly that this issue as not at the order of magnitude we see with other messages, for example we have a total of 8,273 samples in PSpeechSynthesis::Msg_ReadVoicesAndState, while PAPZCTreeManager::Msg_ReceiveMouseInputEvent has 347,140,334 samples. We really should only be passing this message in cases where speech synthesis is used and not in every content process, so just making sure that's the case. I think it is.

2. What do you mean by "send it synchronously in the command line"?

Thanks!

Comment 3

[(no longer active)]

(In reply to Eitan Isaacson [:eeejay] from comment #2)
> Ehsan, Two questions:
> 
> 1. Am I reading the telemetry data. Not minimizing this problem, but do I
> understand correctly that this issue as not at the order of magnitude we see
> with other messages, for example we have a total of 8,273 samples in
> PSpeechSynthesis::Msg_ReadVoicesAndState, while
> PAPZCTreeManager::Msg_ReceiveMouseInputEvent has 347,140,334 samples. We
> really should only be passing this message in cases where speech synthesis
> is used and not in every content process, so just making sure that's the
> case. I think it is.

I think a couple of words from your first sentence are missing?  From the rest of the paragraph it seems you are asking about hwy there are so few samples?  If that's the question, the reason is that we don't encounter more messages of this type (for example in case webspeech isn't used much yet.)
 
> 2. What do you mean by "send it synchronously in the command line"?

See the fix in bug 1303096 for example.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

FWIW, I think using command line for this kind of stuff is totally horrible hack. 
It pollutes tools like 'ps ax' quite nicely.

I'm pretty sure we can send this data asynchronously later.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1337077 - Remove sync state transfer of SpeechSynthesis.

Review commit: https://reviewboard.mozilla.org/r/114962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114962/

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to :Ehsan Akhgari from comment #3)
> (In reply to Eitan Isaacson [:eeejay] from comment #2)
> > Ehsan, Two questions:
> > 
> > 1. Am I reading the telemetry data. Not minimizing this problem, but do I
> > understand correctly that this issue as not at the order of magnitude we see
> > with other messages, for example we have a total of 8,273 samples in
> > PSpeechSynthesis::Msg_ReadVoicesAndState, while
> > PAPZCTreeManager::Msg_ReceiveMouseInputEvent has 347,140,334 samples. We
> > really should only be passing this message in cases where speech synthesis
> > is used and not in every content process, so just making sure that's the
> > case. I think it is.
> 
> I think a couple of words from your first sentence are missing?  From the
> rest of the paragraph it seems you are asking about hwy there are so few
> samples?  If that's the question, the reason is that we don't encounter more
> messages of this type (for example in case webspeech isn't used much yet.)
>  

Sorry for the garbled language. Just wanted to make sure that the only people hitting this are the minority of users who use speech synthesis. Looks like that is the case.

Comment 7

[(no longer active)]

(In reply to Eitan Isaacson [:eeejay] from comment #6)
> (In reply to :Ehsan Akhgari from comment #3)
> > (In reply to Eitan Isaacson [:eeejay] from comment #2)
> > > Ehsan, Two questions:
> > > 
> > > 1. Am I reading the telemetry data. Not minimizing this problem, but do I
> > > understand correctly that this issue as not at the order of magnitude we see
> > > with other messages, for example we have a total of 8,273 samples in
> > > PSpeechSynthesis::Msg_ReadVoicesAndState, while
> > > PAPZCTreeManager::Msg_ReceiveMouseInputEvent has 347,140,334 samples. We
> > > really should only be passing this message in cases where speech synthesis
> > > is used and not in every content process, so just making sure that's the
> > > case. I think it is.
> > 
> > I think a couple of words from your first sentence are missing?  From the
> > rest of the paragraph it seems you are asking about hwy there are so few
> > samples?  If that's the question, the reason is that we don't encounter more
> > messages of this type (for example in case webspeech isn't used much yet.)
> >  
> 
> Sorry for the garbled language. Just wanted to make sure that the only
> people hitting this are the minority of users who use speech synthesis.
> Looks like that is the case.

Yeah.

(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #4)
> FWIW, I think using command line for this kind of stuff is totally horrible
> hack. 
> It pollutes tools like 'ps ax' quite nicely.
> 
> I'm pretty sure we can send this data asynchronously later.

We also have SetXPCOMProcessAttributes() which can be used here, I think.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8840476 [details]
Bug 1337077 - Remove sync state transfer of SpeechSynthesis.

https://reviewboard.mozilla.org/r/114962/#review116476

and once we get voices, synthesis starts.
So the only case this might break is if one doesn't use API as expected and relies on there to be voices available when SpeechSynthesis is created.
Since Chrome seems to have the behavior this patch gives, this should be quite safe.

::: dom/media/webspeech/synth/nsSynthVoiceRegistry.h:58
(Diff revision 1)
>    static nsSynthVoiceRegistry* GetInstance();
>  
>    static already_AddRefed<nsSynthVoiceRegistry> GetInstanceForService();
>  
> +  static void RecvInitialVoicesAndState(const nsTArray<RemoteVoice>& aVoices,
> +                                 const nsTArray<nsString>& aDefaults,

odd indentation

::: dom/media/webspeech/synth/nsSynthVoiceRegistry.cpp:242
(Diff revision 1)
>    }
>  
> -  *aIsSpeaking = IsSpeaking();
> +  gSynthVoiceRegistry->mIsSpeaking = aIsSpeaking;
> +
> +  if (aVoices.Length()) {
> +    gSynthVoiceRegistry->NotifyVoicesChanged();

ah, ok, this ends up dispatching the event.

Comment 9

[(no longer active)]

You should also have a change to sync-messages.ini for the message you're removing.

Kan-Ru, do we fail the build if someone forgets to do that?  If not, we probably should do that.

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 8840476 [details]
Bug 1337077 - Remove sync state transfer of SpeechSynthesis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114962/diff/1-2/

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 8840476 [details]
Bug 1337077 - Remove sync state transfer of SpeechSynthesis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114962/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ef637e0cf20
Remove sync state transfer of SpeechSynthesis. r=smaug

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to :Ehsan Akhgari from comment #9)
> You should also have a change to sync-messages.ini for the message you're
> removing.
> 
> Kan-Ru, do we fail the build if someone forgets to do that?  If not, we
> probably should do that.

Yes, that will detected.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ef637e0cf20