Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
428473005:
[XHR] Reduce 'readystatechange' event firing. (Closed)
Description[XHR] Reduce 'readystatechange' event firing.
Currently too many readystatechange events are fired and
it hurts performance.
BUG=336066
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179381
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : rebase #Patch Set 9 : #Messages
Total messages: 23 (0 generated)
I found some layout tests fail. Please wait a while...
https://codereview.chromium.org/428473005/diff/20001/Source/core/xml/XMLHttpR... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/428473005/diff/20001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:428: now - m_previousProgressFireTime >= progressFireInterval || m_previousProgressRemainLength + length >= progressFireMinimumSize; one line? https://codereview.chromium.org/428473005/diff/20001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:437: dispatchProgressEventFromSnapshot(EventTypeNames::progress); progress events are already throttled. the algorithm you're introducing is different from one used by ProgressEventThrottler but seems good enough for keeping apps that depend on extra dispatching of readystatechange events. we could just apply shouldFire to dispatchReadyStateChangeEvent() call but not here. i'm concerned a little with inconsistency of timing between progressevents and readystatechange events. hopefully as I suggested in the bug the frequency and timing of extra readystatechange events should be limited but aligned exactly to one of progressevent. do you think it's feasible?
https://codereview.chromium.org/428473005/diff/20001/Source/core/xml/XMLHttpR... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/428473005/diff/20001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:428: now - m_previousProgressFireTime >= progressFireInterval || m_previousProgressRemainLength + length >= progressFireMinimumSize; On 2014/07/29 11:39:37, tyoshino wrote: > one line? Done. https://codereview.chromium.org/428473005/diff/20001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:437: dispatchProgressEventFromSnapshot(EventTypeNames::progress); On 2014/07/29 11:39:37, tyoshino wrote: > progress events are already throttled. the algorithm you're introducing is > different from one used by ProgressEventThrottler but seems good enough for > keeping apps that depend on extra dispatching of readystatechange events. we > could just apply shouldFire to dispatchReadyStateChangeEvent() call but not > here. > > i'm concerned a little with inconsistency of timing between progressevents and > readystatechange events. hopefully as I suggested in the bug the frequency and > timing of extra readystatechange events should be limited but aligned exactly to > one of progressevent. do you think it's feasible? Done. As discussed, we don't have to care about the consistency between these two events.
lgtm https://codereview.chromium.org/428473005/diff/80001/Source/core/xml/XMLHttpR... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/428473005/diff/80001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:432: // behavior was needed when the progress event was not available. Please replace "This behavior" with "This extra invocation of readystatechange event in LOADING state"
https://codereview.chromium.org/428473005/diff/80001/Source/core/xml/XMLHttpR... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/428473005/diff/80001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:432: // behavior was needed when the progress event was not available. On 2014/07/30 08:44:07, tyoshino wrote: > Please replace > "This behavior" with > "This extra invocation of readystatechange event in LOADING state" Done.
+tkent for OWNER review
lgtm https://codereview.chromium.org/428473005/diff/120001/Source/core/xml/XMLHttp... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/428473005/diff/120001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:68: namespace { anonymous namespace is unnecessary. |const| puts the symbol to internal linkage. https://codereview.chromium.org/428473005/diff/120001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:70: const double readyStateChangeFireInterval = 0.05; // 0.05s Why 0.05s? This needs a comment.
https://codereview.chromium.org/428473005/diff/120001/Source/core/xml/XMLHttp... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/428473005/diff/120001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:68: namespace { On 2014/07/31 04:40:37, tkent wrote: > anonymous namespace is unnecessary. > |const| puts the symbol to internal linkage. Done. https://codereview.chromium.org/428473005/diff/120001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:70: const double readyStateChangeFireInterval = 0.05; // 0.05s On 2014/07/31 04:40:37, tkent wrote: > Why 0.05s? This needs a comment. Done.
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/428473005/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20015)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20034)
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/428473005/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20045)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/428473005/160001
Message was sent while issue was closed.
Change committed as 179381
Message was sent while issue was closed.
A revert of this CL (patchset #9) has been created in https://codereview.chromium.org/486133002/ by yhirano@chromium.org. The reason for reverting is: This change breaks legacy applications.. |
