Closed
Bug 1337077
Opened 7 years ago
Closed 7 years ago
PSpeechSynthesis::Msg_ReadVoicesAndState can take a long time
Categories
(Core :: Web Speech, defect)
Core
Web Speech
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: ehsan.akhgari, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
<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 2•7 years ago
|
||
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!
Flags: needinfo?(eitan) → needinfo?(ehsan)
Reporter | ||
Comment 3•7 years ago
|
||
(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.
Flags: needinfo?(ehsan)
Comment 4•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 6•7 years ago
|
||
(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.
Reporter | ||
Comment 7•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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.
Attachment #8840476 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 9•7 years ago
|
||
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.
Flags: needinfo?(kchen)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
Pushed by eisaacson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1ef637e0cf20 Remove sync state transfer of SpeechSynthesis. r=smaug
Comment 13•7 years ago
|
||
(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.
Flags: needinfo?(kchen)
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1ef637e0cf20
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•